Add not_so_dummy CPFP #302

danielabrozzoni · 2021-10-27T18:28:49Z

Warning: for testing this PR you need to re-generate your CPFP descriptors using mscompiler!

Still TODO:

Proper fee estimation! We know that core says "idk ¯_(ツ)_/¯" quite often, let's estimate the fees by ourselves instead of doing nothing
a cpfp RPC command to manually cpfp a spend/unvault, or maybe a prioritize to change the priority status is enough

src/daemon/database/actions.rs

danielabrozzoni · 2021-10-27T21:07:37Z

~~Argh TODO: fix unit tests... 😭~~ Done :)

src/daemon/database/interface.rs

darosior

First incomplete pass at reviewing the diff. Only reviewed the first 3 commits and quickly skimmed through the 4th one.

src/daemon/bitcoind/interface.rs

src/daemon/bitcoind/poller.rs

src/daemon/bitcoind/interface.rs

src/daemon/control.rs

src/daemon/bitcoind/poller.rs

To be changed once my PRs are merged

...cpfp_secret in revaultd The CPFP private key can be stored in a file called `cpfp_secret` in the datadir. If the `cpfp_secret` file is not found, CPFP will be disabled. If it's found with an invalid key or a key not present in the descriptor, we error. The CPFP descriptor is imported directly in Bitcoin Core, as the revault_tx update modifies it to be a `multi`. This commit introduces code for creating a new wallet, and some functions for querying it. We also create wallets with blank=true, as we don't want them to have private keys. This commit also renames the `KeyError` enum to `NoiseKeyError`, and moves the "Not a fresh wallet..." debug message out of the loop, so we don't spam the logs.

Store the CPFP priority in the database, and ask it in setspendtx. If no priority is passed, false is used. Also, return an error if priority is asked for, but we don't have a cpfp key. Also adds some utilities for retrieving prioritized txs from the db

Add utils for creating, signing and finalizing a CPFP transaction. Also adds a dummy function for fee estimation, that will need to be fixed (we don't have a proper fallback fee).

At every new tip, checks if we have unvaults/spends that have priority and are still unconfirmed. If yes, broadcasts a CPFP transaction. There's no need to explicitly check if we already CPFPed a certain transaction, as if we did, our new CPFP will either RBF the previous one or won't be accepted by the mempool

Also adds the `with_cpfp` flag to disable the CPFP in tests where it's not needed

Testing the CPFP functionality and setspendtx if the CPFP key is missing

It actually happens quite frequently, when the CSV is not satisfied

darosior · 2021-12-11T18:27:11Z

a cpfp RPC command to manually cpfp a spend/unvault, or maybe a prioritize to change the priority status is enough

Maybe this could just be a parameter to updatespendtx?
EDIT: not so sure, or we should make the actual spend parameter optional then.

danielabrozzoni · 2021-12-11T18:52:05Z

Mh, I don't like it that much, updatespendtx is meant to be used for updating the actual spend with more signatures before broadcast, and prioritize should be set at broadcast time or afterwards, so I think a new api would be cleaner.
Anyways, I'll open an issue for the remaining todos when the PR is merged, so we can discuss it there :)

darosior

I have a number of remarks. I've made them a followup so we don't get stuck on this another two months. See #332 .

Some nits here aren't part of my followup.

ACK 2699a1b -- not based on the code but because of the followup in #322.

tests/test_misc.py

tests/test_rpc.py

darosior · 2021-12-12T13:59:32Z

src/daemon/bitcoind/poller.rs

+    let revaultd = revaultd.read().unwrap();
+    let txids: Vec<_> = tx_package.iter().map(|s| s.txid()).collect();
+    let tx_feerate = CpfpableTransaction::max_package_feerate(&tx_package) * 1000; // to sats/kWU
+    if current_feerate < tx_feerate {


Should be at least <= (you won't CPFP if it's already at the right feerate!) and at best have some slack. You probably don't want to pay the cost of a CPFP transaction for a 1 or 2 sats/vb. Maybe CPFP if more than 5sats/vb of difference?

Another, more important, point is that you are considering the set of transactions (which are not linked, so not really a package in the common usage of the term) when computing the feerate. I think it is incorrect. You might have a situation like:

Next block feerate is 80sat/vb

tx A pays 20sat/vb

tx B pays 200sat/vb

You won't CPFP tx A because of the high feerate of tx B. Tx B should not have an influence on the decision to feebump tx A in your code, since they are not related (tx B has no influence on the decision to mine tx A for a miner).
I think a better approach would filter the transactions that don't need to be CPFP'd from the list of to_be_cpfped. And return if the list is empty.

src/daemon/database/schema.rs

darosior · 2021-12-12T22:19:35Z

src/daemon/jsonrpc/api/mod.rs

@@ -211,6 +211,7 @@ pub trait RpcApi {
        &self,
        meta: Self::Metadata,
        spend_txid: Txid,
+        priority: Option<bool>,


You need to update the API doc?

darosior · 2021-12-13T09:55:02Z

src/daemon/bitcoind/poller.rs

+    // Then we CPFP our spends/unvaults, if we can
+    if revaultd.read().unwrap().is_manager() {
+        maybe_cpfp_txs(revaultd, bitcoind)?;
+    }


Any reason for trying to CPFP before broadcasting the Spends? One could argue that we should CPFP as soon as possible if high prio: a Spend at say 40sat/vb with a current estimate at 80sat/vb is ngmi until we feebump it.

darosior · 2021-12-13T10:34:25Z

src/daemon/bitcoind/poller.rs

+    let listunspent: Vec<_> = listunspent
+        .into_iter()
+        .filter_map(|l| {
+            // Not considering UTXOs still in mempool
+            if l.confirmations < 1 {
+                None
+            } else {
+                let txout = CpfpTxOut::new(
+                    Amount::from_sat(l.txo.value),
+                    &revaultd.derived_cpfp_descriptor(l.derivation_index.expect("Must be here")),
+                );
+                Some(CpfpTxIn::new(l.outpoint, txout))
+            }
+        })
+        .collect();


Trying to get the derivation index for the package's CPFP outputs from bitcoind will result in not being able to replace an existing CPFP tx.

Say you have a CPFP for tx A and tx B. Next block you need to CPFP for tx A tx B tx C at a higher feerate, you need to be able to see the UTxOs used in the former CPFP to reuse them in the latter (and you wouldn't since your listunspent will consider tx A's and tx B's CPFP outputs as spent already.

Note that it doesn't matter for now since the RBF rules prevent us from replacing with a CPFP adding new unconfirmed inputs. But you already have the information in DB, why bother trying to get it from bitcoind?

EDIT: Actually the Spend CPFP output derivation index makes it harder. Let's leave it as is for now and address later.

src/daemon/database/interface.rs

580ba22 qa: increase HTTP timeout for bitcoind requests (Antoine Poinsot) 19ce7a2 CPFP related code cleanups (Antoine Poinsot) bcebe0a qa: extend CPFP tests (Antoine Poinsot) 7050a96 qa: enhance the CPFP test (Antoine Poinsot) 978d3dd bitcoind: base the decision to CPFP on each tx individual feerate (Antoine Poinsot) b5ae52f bitcoind: batch the Unvault and Spend CPFP txs (Antoine Poinsot) f066dbf bitcoind: reorganize code in cpfp_package() (Antoine Poinsot) f3b92aa bitcoind: abort CPFP, don't crash on listunspent breakage (Antoine Poinsot) 6bbefc7 Update revault_tx (Antoine Poinsot) 03501e5 bitcoind: batch the spends CPFP (Antoine Poinsot) 00e9a83 bitcoind: don't CPFP already confirmed Unvault transactions (Antoine Poinsot) 113db34 bitcoind: remove needless call to 'finalizepsbt' (Antoine Poinsot) afea49f bitcoind: take and return a Psbt in sign_psbt (Antoine Poinsot) Pull request description: This is a followup to #302, fixing inconsistencies and cleaning up a bit. The revault_tx counterpart is at revault/revault_tx#117 . ACKs for top commit: danielabrozzoni: ACK 580ba22 Tree-SHA512: d11c2099b347584a0ffc4942cc53d7c1caeb324913149d4386fb5cd9a30336a4e15ddb5f2c820c953cd934771c08fb56ff29d1bfe41debbae4b878bf25ed8bfe

danielabrozzoni mentioned this pull request Oct 27, 2021

bitcoind: Isolate listunspent method #298

Closed

danielabrozzoni force-pushed the 20211006_cpfp branch 3 times, most recently from 57357ac to 85a4444 Compare October 27, 2021 19:14

danielabrozzoni marked this pull request as ready for review October 27, 2021 19:16

danielabrozzoni commented Oct 27, 2021

View reviewed changes

src/daemon/database/actions.rs Outdated Show resolved Hide resolved

danielabrozzoni force-pushed the 20211006_cpfp branch 2 times, most recently from 23a52fa to 70eeb53 Compare October 29, 2021 19:46

danielabrozzoni commented Nov 2, 2021

View reviewed changes

src/daemon/database/interface.rs Show resolved Hide resolved

danielabrozzoni force-pushed the 20211006_cpfp branch 3 times, most recently from 95c0c76 to 8781c7b Compare November 3, 2021 14:43

danielabrozzoni requested a review from darosior November 3, 2021 14:43

darosior reviewed Nov 4, 2021

View reviewed changes

darosior reviewed Nov 9, 2021

View reviewed changes

src/daemon/bitcoind/poller.rs Outdated Show resolved Hide resolved

src/daemon/bitcoind/poller.rs Outdated Show resolved Hide resolved

src/daemon/bitcoind/poller.rs Outdated Show resolved Hide resolved

src/daemon/bitcoind/poller.rs Outdated Show resolved Hide resolved

danielabrozzoni force-pushed the 20211006_cpfp branch 6 times, most recently from 0a69f5c to 1199102 Compare November 17, 2021 09:45

danielabrozzoni force-pushed the 20211006_cpfp branch 8 times, most recently from 146276c to 2e5f521 Compare December 2, 2021 16:01

danielabrozzoni force-pushed the 20211006_cpfp branch 2 times, most recently from 3b093ba to 818f68a Compare December 10, 2021 11:44

danielabrozzoni changed the title ~~Add dummy CPFP~~ Add not_so_dummy CPFP Dec 10, 2021

danielabrozzoni force-pushed the 20211006_cpfp branch 5 times, most recently from 1e85198 to 84cfaa5 Compare December 10, 2021 14:40

danielabrozzoni requested a review from darosior December 10, 2021 14:40

danielabrozzoni added 12 commits December 10, 2021 19:45

WIP: Update revault_tx, revault_net

18494c6

To be changed once my PRs are merged

bitcoind: Isolate listunspent method

f838d4a

bitcoind: panic on API breaks

5624d26

Core utils for the CPFP transaction, fee estimation

902aa66

Add utils for creating, signing and finalizing a CPFP transaction. Also adds a dummy function for fee estimation, that will need to be fixed (we don't have a proper fallback fee).

tests: Store the CPFP priv_key in cpfp_secret

b834a5d

Also adds the `with_cpfp` flag to disable the CPFP in tests where it's not needed

tests: Pass priority in unvault/spend utilities

1f4b4b7

tests: Add CPFP tests

ea9b8a5

Testing the CPFP functionality and setspendtx if the CPFP key is missing

bitcoind: Remove wrong comment

177a551

It actually happens quite frequently, when the CSV is not satisfied

Commit xpub_converter Cargo.lock

2699a1b

danielabrozzoni force-pushed the 20211006_cpfp branch from 84cfaa5 to 2699a1b Compare December 10, 2021 18:45

darosior mentioned this pull request Dec 13, 2021

CPFP fixups and followups #332

Merged

darosior reviewed Dec 13, 2021

View reviewed changes

darosior mentioned this pull request Dec 13, 2021

RBF our CPFP transactions #333

Merged

darosior merged commit d4d78e3 into revault:master Dec 14, 2021

danielabrozzoni deleted the 20211006_cpfp branch February 28, 2022 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add not_so_dummy CPFP #302

Add not_so_dummy CPFP #302

danielabrozzoni commented Oct 27, 2021 •

edited

Loading

danielabrozzoni commented Oct 27, 2021 •

edited

Loading

darosior left a comment

darosior commented Dec 11, 2021 •

edited

Loading

danielabrozzoni commented Dec 11, 2021

darosior left a comment

darosior Dec 12, 2021

darosior Dec 12, 2021

darosior Dec 13, 2021 •

edited

Loading

darosior Dec 13, 2021

Add not_so_dummy CPFP #302

Add not_so_dummy CPFP #302

Conversation

danielabrozzoni commented Oct 27, 2021 • edited Loading

Warning: for testing this PR you need to re-generate your CPFP descriptors using mscompiler!

Still TODO:

danielabrozzoni commented Oct 27, 2021 • edited Loading

darosior left a comment

Choose a reason for hiding this comment

darosior commented Dec 11, 2021 • edited Loading

danielabrozzoni commented Dec 11, 2021

darosior left a comment

Choose a reason for hiding this comment

darosior Dec 12, 2021

Choose a reason for hiding this comment

darosior Dec 12, 2021

Choose a reason for hiding this comment

darosior Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

darosior Dec 13, 2021

Choose a reason for hiding this comment

danielabrozzoni commented Oct 27, 2021 •

edited

Loading

danielabrozzoni commented Oct 27, 2021 •

edited

Loading

darosior commented Dec 11, 2021 •

edited

Loading

darosior Dec 13, 2021 •

edited

Loading