Skip to content
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

Add not_so_dummy CPFP #302

Merged
merged 12 commits into from
Dec 14, 2021
172 changes: 166 additions & 6 deletions src/daemon/bitcoind/poller.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,22 @@ use crate::daemon::{
},
interface::{
db_broadcastable_spend_transactions, db_cancel_dbtx, db_canceling_vaults,
db_emering_vaults, db_exec, db_spending_vaults, db_tip, db_unemering_vaults,
db_unvault_dbtx, db_unvault_transaction, db_vault_by_deposit, db_vault_by_unvault_txid,
db_vaults_dbtx, db_wallet,
db_cpfpable_spends, db_cpfpable_unvaults, db_emering_vaults, db_exec,
db_spending_vaults, db_tip, db_unemering_vaults, db_unvault_dbtx,
db_unvault_transaction, db_vault_by_deposit, db_vault_by_unvault_txid, db_vaults_dbtx,
db_wallet,
},
schema::DbVault,
},
revaultd::{BlockchainTip, RevaultD, VaultStatus},
};
use revault_tx::{
bitcoin::{secp256k1::Secp256k1, Amount, OutPoint, Txid},
error::TransactionCreationError,
miniscript::descriptor::{DescriptorSecretKey, DescriptorXKey, KeyMap, Wildcard},
transactions::{RevaultTransaction, UnvaultTransaction},
txins::RevaultTxIn,
txouts::RevaultTxOut,
transactions::{CpfpableTransaction, RevaultTransaction, UnvaultTransaction},
txins::{CpfpTxIn, RevaultTxIn},
txouts::{CpfpTxOut, RevaultTxOut},
};

use std::{
Expand Down Expand Up @@ -391,6 +393,159 @@ fn mark_confirmed_emers(
Ok(())
}

fn cpfp_package(
revaultd: &Arc<RwLock<RevaultD>>,
bitcoind: &BitcoinD,
mut tx_package: Vec<impl CpfpableTransaction>,
current_feerate: u64,
) -> Result<(), BitcoindError> {
let revaultd = revaultd.read().unwrap();
let txids: Vec<_> = tx_package.iter().map(|s| s.txid()).collect();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are constructing this only for searching through it you want a set (O(1) lookups), not a vec (O(n) lookups).
https://doc.rust-lang.org/std/collections/index.html#performance

let tx_feerate = CpfpableTransaction::max_package_feerate(&tx_package) * 1000; // to sats/kWU
if current_feerate < tx_feerate {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Uhm, we don't need to cpfp this for now, our estimate is lower
// than the tx fees.
log::debug!("Txs '{:?}' don't need CPFP", txids);
return Ok(());
}

let added_feerate = current_feerate - tx_feerate;
let listunspent: Vec<_> = bitcoind.list_unspent_cpfp()?;

// FIXME: drain_filter would be PERFECT for this but it's nightly only :(
let (mut my_listunspent, listunspent): (Vec<_>, Vec<_>) = listunspent
.into_iter()
.partition(|l| txids.contains(&l.outpoint.txid));

if my_listunspent.len() != tx_package.len() {
log::warn!(
"We need to feebump a package containing the following txids: {:?},\n
but we found listunspents only for {:?}",
txids,
my_listunspent
.iter()
.map(|l| l.outpoint.txid)
.collect::<Vec<_>>()
);
}

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();
Comment on lines +431 to +445
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


my_listunspent.sort_by_key(|l| l.outpoint.txid);
tx_package.sort_by_key(|tx| tx.txid());

// I can do this as I just ordered by txid
let tx_package: Vec<_> = tx_package
.into_iter()
.enumerate()
.map(|(i, p)| {
let derived_cpfp_descriptor = revaultd
.derived_cpfp_descriptor(my_listunspent[i].derivation_index.expect("Must be here"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will panic if both vectors don't have the same size. You check this above, but don't return. Therefore it can be triggered.

(p, derived_cpfp_descriptor)
})
.collect();

let psbt = match CpfpableTransaction::cpfp_transactions(&tx_package, added_feerate, listunspent)
{
Ok(tx) => tx,
Err(TransactionCreationError::InsufficientFunds) => {
// Well, we're poor.
log::error!(
"We wanted to feebump transactions '{:?}', but we don't have enough funds!",
txids
);
return Ok(());
}
Err(e) => {
log::error!("Error while creating CPFP transaction: '{}'", e);
return Ok(());
}
};

let psbt = psbt.as_psbt_string();

let (complete, psbt_signed) = bitcoind.sign_psbt(psbt)?;
if !complete {
log::error!(
"Bitcoind returned a non-finalized CPFP PSBT: {}",
psbt_signed
);
return Ok(());
}
let psbt_signed = bitcoind.finalize_psbt(psbt_signed)?;

if let Err(e) = bitcoind.broadcast_transaction(&psbt_signed) {
log::error!("Error broadcasting '{:?}' CPFP tx: {}", txids, e);
} else {
log::info!("CPFPed transactions with ids '{:?}'", txids);
}

Ok(())
}

fn maybe_cpfp_txs(
revaultd: &Arc<RwLock<RevaultD>>,
bitcoind: &BitcoinD,
) -> Result<(), BitcoindError> {
let db_path = revaultd.read().unwrap().db_file();
log::debug!("Checking if transactions need CPFP...");

if revaultd.read().unwrap().cpfp_key.is_none() {
log::warn!("We should CPFP transactions, but we don't have a cpfp key!");
return Ok(());
}
darosior marked this conversation as resolved.
Show resolved Hide resolved

let current_feerate = match bitcoind.estimate_feerate()? {
Some(f) => f,
None => {
log::warn!("Fee estimation not available, skipping CPFP");
return Ok(());
}
};

let spend_txs: Vec<_> = db_cpfpable_spends(&db_path)?;
let unvault_packages: Vec<_> = db_cpfpable_unvaults(&db_path)?;
for spend_tx in spend_txs {
// We check if this transaction is still unconfirmed. If so, we feebump
if bitcoind
.get_wallet_transaction(&spend_tx.txid())?
.blockheight
.is_none()
{
// As cpfp_package expects a package, we wrap our spend_tx in a Vec
cpfp_package(revaultd, bitcoind, vec![spend_tx], current_feerate)?;
}
}
Comment on lines +521 to +531
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you are going to create one CPFP tx per Spend?! You have the code to handle a list of transactions why incurring the cost of multiple onchain transactions to the user?


for unvault_package in unvault_packages {
// We check if any unvault in this package is still unconfirmed. If so, we feebump
if unvault_package.iter().any(|u| {
bitcoind
.get_wallet_transaction(&u.txid())
.map(|w| w.blockheight.is_none())
.unwrap_or(true)
Comment on lines +533 to +539
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you would CPFP already confirmed transactions?..

}) {
cpfp_package(revaultd, bitcoind, unvault_package, current_feerate)?;
}
Comment on lines +533 to +542
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using a single CPFP?

}
Comment on lines +521 to +543
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get why you create N transactions while you have all the data to create one. This has only drawbacks. And, to the risk of stating the obvious, "it makes code slightly less complicated since we don't need an enum" isn't a valid rationale for wasting users' funds.

Copy link
Collaborator Author

@danielabrozzoni danielabrozzoni Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a valid rationale, I had reasons in the past for doing so, when refactoring the code for the tenth time in a row I overlooked that those assumptions don't hold anymore. Sorry.

Can you please stop posting multiple comments for the same problem? It's the second comment stating this same thing (and I could probably find many more examples in this same PR). If you rethink your statement after clicking the Add comment just click edit...

Edit: it's actually the third comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, multiple comments isn't to piss you off. Just the two first were organic comments as read along the code and this one is more of a "global" comment englobing the two. True that i could have deleted the two first comments, if it hurts


Ok(())
}

// Everything we do when the chain moves forward
fn new_tip_event(
revaultd: &Arc<RwLock<RevaultD>>,
Expand All @@ -403,6 +558,11 @@ fn new_tip_event(
// First we update it in DB
db_update_tip(&db_path, new_tip)?;

// Then we CPFP our spends/unvaults, if we can
if revaultd.read().unwrap().is_manager() {
maybe_cpfp_txs(revaultd, bitcoind)?;
}
Comment on lines +560 to +563
Copy link
Member

@darosior darosior Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Then we check if any Spend became mature yet
maybe_broadcast_spend_transactions(revaultd, bitcoind)?;

Expand Down