-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
commands: add rbfpsbt
command
#816
Conversation
859e6e0
to
b3e143d
Compare
c2b8f47
to
add65fe
Compare
src/commands/mod.rs
Outdated
// sum of fees paid by original transactions + incremental feerate * replacement size. | ||
for i in 0..100_u64 { | ||
println!("trying feerate of {} with i {}", feerate_vb + i, i); | ||
let rbf_psbt = self.create_rbf_spend( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case change_index
is None
, I can pass an arbitrary value here to avoid incrementing the change index while we try different feerates, and then once the necessary feerate has been found, we can do a final call that increments the change index if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this initial idea, I instead pass the next change address directly to create_spend_internal
. It might be that we still increment the change index once in an intermediate call and then the final transaction won't have change, but this approach seems simpler.
add65fe
to
4a3e657
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit baffled by your iterating approach to get the minimum absolute fee.
src/commands/mod.rs
Outdated
.ok_or(CommandError::InsaneFees(InsaneFeeInfo::InvalidFeerate))? | ||
}; | ||
// Check replacement transaction's feerate is greater than previous feerate (the only directly conflicting transaction). | ||
// TODO: Should we throw error or just set feerate_vb = prev_feerate_vb + 1 and check `rbf_psbt` below? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think:
- We should let them provide a feerate for fee bumps. In this case, we should explicitly error if they set a too low feerate.
- We should not require a feerate for cancels. In this case we should set the feerate to the minimum necessary to both not lower the absolute fee and increment the feerate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some logic along these lines, although it might need some tweaking.
4a3e657
to
fff63a3
Compare
I rebased on master and updated the functional test for the new Other changes relating to the comments above are still ongoing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
2c29cd5
to
8ee4112
Compare
3e2d37e
to
19a1b2d
Compare
19a1b2d
to
ac5a00c
Compare
rbfpsbt
commandrbfpsbt
command
.filter_map(|c| { | ||
// In case the user attempts RBF before the previous coins have been updated in the DB, | ||
// they would be returned as confirmed and not spending. | ||
if !prev_coins.contains_key(&c.outpoint) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative would be to wait for the spend info of all coins to be updated.
// transaction, then set all previous coins as mandatory and add confirmed coins as | ||
// optional, unless we have already done this. | ||
Err(CommandError::CoinSelectionError(_)) | ||
if is_cancel && candidate_coins.iter().all(|c| !c.must_select) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative approach later on might be for coin selection to pick at least one from a subset. E.g. we could use a metric that only returns a score if the selection includes at least one candidate from this subset. So for the cancel case, we would want at least one from the previous inputs, and then zero or more from the optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't looked at the tests yet. This is looking good, i like the create_spend_internal interface, definitely paving the way for completely abstracting out the spend creation. Nice commits organization too, although i'd have made a simple single commit of 5fe6ae9 and 41d6a4b so it can be reviewed using --color-moved=dimmed-zebra
.
src/commands/mod.rs
Outdated
// sanity check each output's value. | ||
let mut psbt_outs = Vec::with_capacity(destinations.len()); | ||
let mut destinations_checked: HashMap<bitcoin::Address, bitcoin::Amount> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for the type declaration here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed this and a similar one in rbf_psbt()
.
src/commands/mod.rs
Outdated
.iter() | ||
.filter_map(|(addr, amt, _)| { | ||
if prev_change_address != Some(addr) { | ||
Some((addr.clone(), *amt)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use into_iter() and avoid this clone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a good idea to move prev_change_address
's cloned()
from below to just above if you do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've now made this change.
Just realized that actually this doesn't support RBF'ing a recovery transaction as in this case we need to set the right sequence in the inputs. |
I've been working on top of this branch and been running the functional tests a lot. Noticed a somewhat flaky RBF test. FYI: s...............................F.. [100%]
=============================================================================================== FAILURES ===============================================================================================
_________________________________________________________________________________________ test_rbfpsbt_cancel __________________________________________________________________________________________
[gw9] linux -- Python 3.11.6 /home/darosior/projects/wizardsardine/liana/venv/bin/python3
lianad = <test_framework.lianad.Lianad object at 0x7fc4a0ad71d0>, bitcoind = <test_framework.bitcoind.Bitcoind object at 0x7fc493f33090>
def test_rbfpsbt_cancel(lianad, bitcoind):
"""Test the use of RBF to cancel a transaction."""
# Get three coins.
destinations = {
lianad.rpc.getnewaddress()["address"]: 0.003,
lianad.rpc.getnewaddress()["address"]: 0.004,
lianad.rpc.getnewaddress()["address"]: 0.005,
}
txid = bitcoind.rpc.sendmany("", destinations)
bitcoind.generate_block(1, wait_for_mempool=txid)
wait_for(lambda: len(lianad.rpc.listcoins(["confirmed"])["coins"]) == 3)
coins = lianad.rpc.listcoins(["confirmed"])["coins"]
# Create a spend that will later be replaced.
first_outpoints = [c["outpoint"] for c in coins[:2]]
destinations = {
bitcoind.rpc.getnewaddress(): 650_000,
}
first_res = lianad.rpc.createspend(destinations, first_outpoints, 1)
first_psbt = PSBT.from_base64(first_res["psbt"])
# The transaction has a change output.
assert len(first_psbt.o) == len(first_psbt.tx.vout) == 2
first_txid = first_psbt.tx.txid().hex()
# Broadcast the spend and wait for it to be detected.
first_txid = sign_and_broadcast_psbt(lianad, first_psbt)
wait_for(
lambda: all(
c["spend_info"] is not None and c["spend_info"]["txid"] == first_txid
for c in lianad.rpc.listcoins([], first_outpoints)["coins"]
)
)
# We can use RBF and let the command choose the min possible feerate (1 larger than previous).
rbf_1_res = lianad.rpc.rbfpsbt(first_txid, True)
# We can also pass the feerate explicitly.
rbf_1_res = lianad.rpc.rbfpsbt(first_txid, True, 2)
rbf_1_psbt = PSBT.from_base64(rbf_1_res["psbt"])
# Replacement only has a single input.
assert len(rbf_1_psbt.i) == 1
# This input is one of the two from the previous transaction.
assert rbf_1_psbt.i[0].map[PSBT_IN_NON_WITNESS_UTXO] in [
psbt_in.map[PSBT_IN_NON_WITNESS_UTXO] for psbt_in in rbf_1_psbt.i
]
# The replacement only has a change output.
assert len(rbf_1_psbt.tx.vout) == 1
# Change address is the same but change amount will be higher in the replacement as it is the only output.
assert first_psbt.tx.vout[1].nValue < rbf_1_psbt.tx.vout[0].nValue
assert first_psbt.tx.vout[1].scriptPubKey == rbf_1_psbt.tx.vout[0].scriptPubKey
# Broadcast the replacement and wait for it to be detected.
rbf_1_txid = sign_and_broadcast_psbt(lianad, rbf_1_psbt)
# The spend info of the coin used in the replacement will be updated.
rbf_1_outpoint = (
f"{rbf_1_psbt.tx.vin[0].prevout.hash:x}:{rbf_1_psbt.tx.vin[0].prevout.n}"
)
> assert rbf_1_outpoint in first_outpoints
E AssertionError: assert '389e33bec707e74f00650f337f576e236eb3a088c36c6999cf148698c9ba751:1' in ['0389e33bec707e74f00650f337f576e236eb3a088c36c6999cf148698c9ba751:1', '0389e33bec707e74f00650f337f576e236eb3a088c36c6999cf148698c9ba751:3']
tests/test_rpc.py:1210: AssertionError
---------------------------------------------------------------------------------------- Captured stdout setup -----------------------------------------------------------------------------------------
Running tests in /tmp/lianad-tests-up_02ari
--------------------------------------------------------------------------------------- Captured stdout teardown ---------------------------------------------------------------------------------------
Test failed, leaving directory '/tmp/lianad-tests-up_02ari/test_rbfpsbt_cancel_1' intact
Leaving base dir '/tmp/lianad-tests-up_02ari' as it still contains ['test_rbfpsbt_cancel_1']
======================================================================================= short test summary info ========================================================================================
FAILED tests/test_rpc.py::test_rbfpsbt_cancel - AssertionError: assert '389e33bec707e74f00650f337f576e236eb3a088c36c6999cf148698c9ba751:1' in ['0389e33bec707e74f00650f337f576e236eb3a088c36c6999cf148698c9ba751:1', '0389e33bec707e74f00650f337f57...
=============================================================================== 1 failed, 33 passed, 1 skipped in 35.95s =============================================================================== |
Thanks for spotting the flaky test. I think the following should fix that error: diff --git a/tests/test_rpc.py b/tests/test_rpc.py
index 6b3004b..69ecce3 100644
--- a/tests/test_rpc.py
+++ b/tests/test_rpc.py
@@ -1203,7 +1203,7 @@ def test_rbfpsbt_cancel(lianad, bitcoind):
# The spend info of the coin used in the replacement will be updated.
rbf_1_outpoint = (
- f"{rbf_1_psbt.tx.vin[0].prevout.hash:x}:{rbf_1_psbt.tx.vin[0].prevout.n}"
+ f"{rbf_1_psbt.tx.vin[0].prevout.hash:064x}:{rbf_1_psbt.tx.vin[0].prevout.n}"
)
assert rbf_1_outpoint in first_outpoints The left-hand value in your error is missing the leading zero from the right-hand side, so this change should ensure the value is padded with zeros. |
ac5a00c
to
01d50c9
Compare
I've now done this. |
I've now addressed the comments. |
@edouardparis can you have a quick look at 01d50c9? |
@@ -171,6 +173,19 @@ pub enum InsaneFeeInfo { | |||
TooHighFeerate(u64), | |||
} | |||
|
|||
#[derive(Debug, Clone, Copy, PartialEq, Eq)] | |||
pub enum RbfErrorInfo { | |||
TooLowFeerate(u64), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be helpful to include the min feerate here as well as / instead of the chosen feerate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't changed this yet. We could leave for a follow-up.
src/bitcoin/d/mod.rs
Outdated
let fees_json = json | ||
.get("fees") | ||
.expect("Must be present in bitcoind response") | ||
.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit (to avoid the clone, feel free to ignore):
diff --git a/src/bitcoin/d/mod.rs b/src/bitcoin/d/mod.rs
index c993813..8c52a1b 100644
--- a/src/bitcoin/d/mod.rs
+++ b/src/bitcoin/d/mod.rs
@@ -1413,15 +1413,13 @@ impl From<Json> for MempoolEntry {
.get("vsize")
.and_then(Json::as_u64)
.expect("Must be present in bitcoind response");
- let fees_json = json
+ let fees = json
.get("fees")
+ .as_ref()
.expect("Must be present in bitcoind response")
- .clone();
+ .into();
- MempoolEntry {
- vsize,
- fees: MempoolEntryFees::from(fees_json),
- }
+ MempoolEntry { vsize, fees }
}
}
@@ -1431,8 +1429,8 @@ pub struct MempoolEntryFees {
pub descendant: bitcoin::Amount,
}
-impl From<Json> for MempoolEntryFees {
- fn from(json: Json) -> MempoolEntryFees {
+impl From<&&Json> for MempoolEntryFees {
+ fn from(json: &&Json) -> MempoolEntryFees {
let json = json.as_object().expect("fees must be an object");
let base = json
.get("base")
| `is_cancel` | bool | Whether to "cancel" the transaction or simply bump the fee. | | ||
| `feerate` | integer(optional) | Target feerate for the RBF transaction (in sat/vb). | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fond of having both as separate parameters since the feerate should always be set if it's not a cancel. But i don't have a better suggestion.
src/commands/mod.rs
Outdated
is_cancel: bool, | ||
feerate_vb: Option<u64>, | ||
) -> Result<CreateSpendResult, CommandError> { | ||
// The transaction to be replaced must be one of ours and so we can assume it signals replaceability. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we could have a sanity check for this since we are already iterating over the inputs of the existing PSBT anyways.
src/commands/mod.rs
Outdated
// Get the spending transactions of the coins from DB in order to determine the transactions | ||
// that are directly conflicting with the replacement transaction we will create. | ||
// Note that the DB may be missing very recently broadcast transactions. | ||
let mut dir_conflict_txids: HashSet<bitcoin::Txid> = prev_coins | ||
.values() | ||
.filter_map(|coin| coin.spend_txid) | ||
.collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be silently ignoring those we haven't yet marked as spent. Or we might create a PSBT which the user would go through the hassle of signing (and potentially getting signed by others) just for it to not be broadcastable in the end. I know i'm walking back from #816 (comment) but giving it more thought i don't think it's reasonable. I can also tackle this in a follow-up.
src/commands/mod.rs
Outdated
// In case this transaction is not in the mempool, we assume that it is finalized when | ||
// calculating its fee and vsize below, i.e. we assume that this RBF command is used only | ||
// on finalized PSBTs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We never store finalized PSBTs in database. You can always try to finalize it to get the exact size and raise an error if you couldn't (which necessarily means the PSBT wasn't broadcast from our wallet):
Lines 761 to 773 in bcfab48
// First, try to finalize the spending transaction with the elements contained | |
// in the PSBT. | |
let mut spend_psbt = db_conn | |
.spend_tx(txid) | |
.ok_or(CommandError::UnknownSpend(*txid))?; | |
spend_psbt.finalize_mut(&self.secp).map_err(|e| { | |
CommandError::SpendFinalization( | |
e.into_iter() | |
.next() | |
.map(|e| e.to_string()) | |
.unwrap_or_default(), | |
) | |
})?; |
Or just using gettxspendingprevout
would avoid the need for finalized PSBTs altogether.
src/commands/mod.rs
Outdated
// Check replacement transaction's target feerate, if set, is high enough, | ||
// and otherwise set it to the min feerate found above. | ||
let feerate_vb = feerate_vb.unwrap_or(min_feerate_vb); | ||
if feerate_vb < min_feerate_vb { | ||
return Err(CommandError::RbfError(RbfErrorInfo::TooLowFeerate( | ||
feerate_vb, | ||
))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should only do that when cancelling. I don't see how not passing a feerate isn't a mistake otherwise. If you want to bump fees, bumping by 1s/vb isn't going to achieve much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conversely, i don't think there is any reason for setting the feerate when using a cancel.
let address = bitcoin::Address::from_script( | ||
&txo.script_pubkey, | ||
self.config.bitcoin_config.network, | ||
) | ||
.expect("address already used in finalized transaction"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't sanity check PSBTs before storing them in DB so technically this might not hold.
I've addressed my comments in separate commits for you to review and squash them in the appropriate ones.
I've also played with this a bit more. I think this is good to go if you agree with my changes. |
prev_coins | ||
}; | ||
if !prev_psbt.unsigned_tx.is_explicitly_rbf() { | ||
return Err(CommandError::RbfError(RbfErrorInfo::NotSignaling)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we rather check all the spending transactions in the mempool? Maybe using the bip125-replaceable
field in getmempoolentry
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that in a follow-up. Note however this is only checked for directly conflicting transactions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the directly conflicting transactions should correspond to the mempool_spenders
for the previous outpoints, but happy to come back to that in a follow-up.
`is_in_mempool` has been updated to use `mempool_entry`.
This is a more general version of `create_spend` in that it allows for a mixure of both mandatory and optional coin selection candidates and has a `min_fee` parameter. Another difference is that it takes destination addresses that have been checked for the network and amounts as `Amount`. `create_spend` has been updated to call `create_spend_internal`.
96f2a5d
to
0ab00cd
Compare
Thanks for your commits. I've now reviewed and squashed them as appropriate. |
ACK 0ab00cd |
0f69411 spend: a nicer interface for providing fee informations (Antoine Poinsot) 990b153 commands: don't query unconfirmed coins when creating recovery tx (Antoine Poinsot) c63a120 spend: don't use database's Coin type (Antoine Poinsot) 08ce0ad spend: update comment about create_spend behaviour (Antoine Poinsot) 0c395bb spend: document the create_spend function (Antoine Poinsot) f3113ba commands: remove redundant output value check (Antoine Poinsot) 5894e78 spend: move tx size calc helper back to command module (Antoine Poinsot) 6ddda61 spend: make coin selection helpers private (Antoine Poinsot) 33be1ff commands: make create_recovery use the create_spend helper (Antoine Poinsot) 0523f00 commands: update next deriv index for any spend output address (Antoine Poinsot) 5d50155 spend: avoid direct access to our Bitcoin backend (Antoine Poinsot) 7c23812 spend: don't access the database in the PSBT creation function (Antoine Poinsot) 22f97e1 spend: let caller update next derivation index (Antoine Poinsot) 9fdb75c commands: split up spend transaction creation into its own module (Antoine Poinsot) Pull request description: Based on top of #816, this introduces a new `spend` module with a helper to create a transaction spending coins from the wallet. It can be leveraged to create regular or recovery transactions, and also replacement for them. All the data structures used by the exposed spend creation function are contained with this module, in order to make it usable without a `lianad`-specific database and Bitcoin interface. This PR is structured in an incremental fashion. First we pull out the `create_spend_internal` method introduced in #816 into a standalone `spend` module, then we incrementally remove the cruft and the ties from the spend module to the other components. ACKs for top commit: darosior: self-ACK 0f69411 Tree-SHA512: a75afeb2c1f58e685c6b6e0d88c53e158ad850266261ef93b20065f6b02ad9e817cefcef4a89dc038e4db81549efc6d5393e4c59b4f7d86b69dc2168b9d818d3
This PR relates to #43 and #236.
It adds a
rbfpsbt
command that generates a PSBT to replace an existing transaction using RBF. This replacement can either preserve non-change outputs and simply bump fees or remove non-change outputs and effectively cancel the transaction. The inputs and change output may need to be updated in accordance with the higher fee.I've also added a
getmempoolentry
call to the bitcoin interface that is used for checking information about descendant transactions.To facilitate development, I've made some temporary changes in the GUI so that replacement PSBTs can be signed and broadcast, but these changes might not be part of this PR in the end.