Fix RBF #292
Fix RBF #292
Conversation
samholmes
force-pushed
the
patch/rbf
branch
2 times, most recently
from
9b83a8f to
0579e9d
Compare
samholmes
force-pushed
the
patch/rbf
branch
2 times, most recently
from
2755479 to
aad42cb
Compare
github-actions
bot
force-pushed
the
patch/rbf
branch
from
aad42cb to
289326b
Compare
samholmes
force-pushed
the
patch/rbf
branch
2 times, most recently
from
c0b4d6d to
e94f6a5
Compare
| @@ -428,7 +427,7 @@ export async function makeUtxoEngine( | |||
| feeRate, | |||
| coin: coinInfo.name, | |||
| currencyCode: currencyInfo.currencyCode, | |||
| setRBF, | |||
| enableRbf: edgeSpendInfo.otherParams?.enableRbf ?? false, | |||
There was a problem hiding this comment.
Could we add a fully-typed EdgeSpendInfo.enableRbfFlag to edge-core-js? That would be cleaner, but I understand that otherParams is acceptable for quick hacks like this.
There was a problem hiding this comment.
Break-away task for this one.
| throw new InsufficientFundsError({ | ||
| currencyCode: args.currencyCode, | ||
| // Repurpose the networkFee property in the error as the fee delta | ||
| networkFee: feeDelta.toString() |
There was a problem hiding this comment.
On the other chains, such as Ethereum, the networkFee is the absolute fee value. If the transaction costs $5 in ETH fees to send, the networkFee is $5 worth of ETH, regardless of the wallet balance. Even if the wallet has $4.99 in ETH, the error's networkFee is still $5, not $0.01.
I may be reading this code wrong, but it seems like you would be returning $0.01 if the transaction costs $5 in fees but the wallet has $4.99 left over after the spend. If that's the case, can we fix this code to return $5 in all situations, for consistency with the other coins?
If that won't work, then maybe we could edit edge-core-js to add an nativeAmountNeeded field to the error, to be clear about the different semantics here.
Is see that you are using this internally as part of your accelerate function, so maybe come up with a custom error type for use by the engine, or find some other way to export the value? Maybe add an Error.otherParams?
|
|
||
| // Double the fee used for the RBF transaction: | ||
| const vBytes = transactionSizeFromHex(replacedTx.hex) | ||
| const newFeeRate = Math.round(parseInt(replacedTx.fees) / vBytes) * 2 |
There was a problem hiding this comment.
Put the * 2 inside the Math.round, so we do the doubling in float-land instead of integer-land.
Rounding creates ±0.5 of "random noise" in the output values, but the * 2 doubles this noise to ±1. If you perform the * 2 before rounding, though, you only get the ±0.5 of noise needed to hit an integer, and nothing more.
| export class InsufficientFundsErrorPlus extends InsufficientFundsError { | ||
| networkFeeShortage?: string | ||
| constructor(opts: InsufficientFundsErrorOptsPlus = {}) { | ||
| super(opts) | ||
| const { networkFeeShortage } = opts | ||
| this.networkFeeShortage = networkFeeShortage | ||
| } | ||
| } |
This is pretty much the only place where this object type is created. Declaring the object on a second line helps with referencing this distinguished place in the code-base.
We cannot depend on the UTXO data to be available in the processor because the UTXO set is pruned as UTXOs are spent. We leverage a new `utxoFromProcessorTransactionInput` function which derives the the UTXO processor data type from a transaction processor data type.
It appears Blockbook omits the `vout` field for inputs where the `vout` is 0 on-chain. I'm not sure why Blockbook does this, but this fixes a bug caused by having a negative `vout`.
This fix calculates the the previous transaction's fee rate before doubling it for RBF. The `IProcessorTransaction` type doesn't include the feeRate, so we can use the altcoin-js library to get the the transaction's vbytes and the transaction's fee amount to calculate the feeRate.
We cannot depend on the UTXO data to be available in the processor because the UTXO set is pruned as UTXOs are spent. Luckily we can by pass the need for the UTXO in the processor by getting the scriptPubkey in the relevant transaction from the processor.
Because we were not awaiting the async function, it was always returning true, and therefore incorrectly including all spend targets into the `ourReceiveAddress` array.
A result without outputs means the picker failed due to insufficient funding by the given UTXOs. Returning the selected inputs is useful to determine the insufficiency of the sum of the input values against the fee.
This is no longer the correct way to do transaction acceleration via RBF. Instead, this is handled by the `accelerate` method.
The rename diminishes the conflation with the defunct `rbfTxid` on `EdgeSpendInfo`.
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
Description
none