wallet: rbf: estimate base tx size before stripping
What the developer wrote
Authored by f321x
wallet: rbf: estimate base tx size before stripping
Estimate the size of a base tx before stripping its signatures
so the lower bound feerate used to calculate the fee for the rbf
transaction doesn't underestimate the feerate of the base tx.
What changed, and why it matters
This commit fixes a fee-calculation bug in Electrum's Replace-By-Fee (RBF) feature. Previously, the wallet estimated the size of the original transaction after removing its signatures. Because removing signatures makes the transaction look smaller than it really is, the wallet could set the new RBF transaction's fee too low, potentially causing the replacement transaction to be rejected by the Bitcoin network for not meeting the required fee rate. The fix estimates the original transaction size before stripping signatures, so the replacement fee is calculated more accurately.
Users relying on RBF/bump-fee should upgrade to a version containing this commit. Developers should review whether any other fee/size estimates in the codebase are taken after stripping signatures or witnesses.
Security signals we found
Fee estimation accuracy bug in RBF path
Transaction size computed after signature stripping
Potential replacement transaction rejected due to insufficient fee
No explicit security framing by vendor
Evidence from the diff
The patch moves calls to tx.estimated_size() to occur before base_tx.remove_signatures() or conversion to PartialTransaction in three wallet methods related to RBF/bump fee functionality. Previously, estimated_size() was called after signatures were stripped, yielding a smaller size estimate. Since fee rate is fee/size, a smaller denominator produces a higher apparent feerate for the base transaction, which could lead the replacement fee estimator to produce a fee for the bumped transaction that does not actually satisfy network relay/minimum replacement feerate requirements. The fix captures old_tx_size before mutation so the lower-bound feerate calculation is based on the real signed transaction size.
Changed components
electrum/wallet.pyRBF / bump fee logicPartialTransaction size estimationInspect captured patch +7 / −6
diff --git a/electrum/wallet.py b/electrum/wallet.py
index 247dfa9..a752341 100644
--- a/electrum/wallet.py
+++ b/electrum/wallet.py
@@ -2019,16 +2019,17 @@ class Abstract_Wallet(ABC, Logger, EventListener):
# make sure we don't try to spend change from the tx-to-be-replaced:
coins = [c for c in coins if c.prevout.txid.hex() != base_tx.txid()]
is_local = self.adb.get_tx_height(base_tx.txid()).height() == TX_HEIGHT_LOCAL
+ # estimate base tx fee before stripping tx for more accurate estimate
+ base_tx_fee = base_tx.get_fee()
+ base_feerate = Decimal(base_tx_fee)/base_tx.estimated_size()
+ relayfeerate = Decimal(self.relayfee()) / 1000
+ original_fee_estimator = fee_estimator
if not isinstance(base_tx, PartialTransaction):
base_tx = PartialTransaction.from_tx(base_tx)
base_tx.add_info_from_wallet(self)
else:
# don't cast PartialTransaction, because it removes make_witness
base_tx.remove_signatures()
- base_tx_fee = base_tx.get_fee()
- base_feerate = Decimal(base_tx_fee)/base_tx.estimated_size()
- relayfeerate = Decimal(self.relayfee()) / 1000
- original_fee_estimator = fee_estimator
def fee_estimator(size: Union[int, float, Decimal]) -> int:
size = Decimal(size)
lower_bound_relayfee = int(base_tx_fee + round(size * relayfeerate)) if not is_local else 0
@@ -2302,6 +2303,7 @@ class Abstract_Wallet(ABC, Logger, EventListener):
Without that, all txins must be ismine.
"""
assert tx
+ old_tx_size = tx.estimated_size() # estimate before stripping tx for more accurate estimate
if not isinstance(tx, PartialTransaction):
tx = PartialTransaction.from_tx(tx)
assert isinstance(tx, PartialTransaction)
@@ -2312,7 +2314,6 @@ class Abstract_Wallet(ABC, Logger, EventListener):
tx.add_info_from_wallet(self)
if tx.is_missing_info_from_network():
raise Exception("tx missing info from network")
- old_tx_size = tx.estimated_size()
old_fee = tx.get_fee()
assert old_fee is not None
old_fee_rate = old_fee / old_tx_size # sat/vbyte
@@ -2572,6 +2573,7 @@ class Abstract_Wallet(ABC, Logger, EventListener):
Without that, all txins must be ismine.
"""
assert tx
+ old_tx_size = tx.estimated_size() # estimate before stripping tx for more accurate estimate
if not isinstance(tx, PartialTransaction):
tx = PartialTransaction.from_tx(tx)
assert isinstance(tx, PartialTransaction)
@@ -2583,7 +2585,6 @@ class Abstract_Wallet(ABC, Logger, EventListener):
tx.add_info_from_wallet(self)
if tx.is_missing_info_from_network():
raise Exception("tx missing info from network")
- old_tx_size = tx.estimated_size()
old_fee = tx.get_fee()
assert old_fee is not None
old_fee_rate = old_fee / old_tx_size # sat/vbyte
Why this scored 37/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.