Feat: automatic rbf by sander2 · Pull Request #313 · interlay/interbtc-clients

sander2 · 2022-07-22T17:20:35Z

I would like to have automated tests but since this really depends on the bitcoin core specifics maybe the best place would be in the lib..

vault/src/execution.rs

bitcoin/src/lib.rs

runtime/src/rpc.rs

gregdhill · 2022-07-27T15:06:19Z

vault/src/execution.rs

+                Either::Left((result, _)) => result?,
+                Either::Right((Some(Ok((old_fee, new_fee))), _)) => {
+                    tracing::info!("Bumping fee rate from {old_fee} to {new_fee}...");
+                    txid = self.bump_fee(btc_rpc, txid, new_fee).await?;


Do we always want to bump the fee as soon as the estimate is higher that the current fee? If it's inexpensive or free I don't see why not but just want to confirm.

Good point. Bumping fees does not cost the client anything, but there is a minimum increase for the network to accept the new tx. I believe that is set to 1 sat/vbyte, and since our price estimate has a granularity of 1 sat/vbyte, we could always bump. But in case 1 of these assumptions fails to hold (in the future) I will add an additional check for it

I decided to just always try and catch the error

gregdhill · 2022-07-27T15:11:32Z

I would like to have automated tests but since this really depends on the bitcoin core specifics maybe the best place would be in the lib..

As much as I would like to shift the burden of responsibility I do think we're best placed to test this, besides we already have some integration tests here.

sander2 · 2022-07-29T17:00:12Z

I would like to have automated tests but since this really depends on the bitcoin core specifics maybe the best place would be in the lib..

As much as I would like to shift the burden of responsibility I do think we're best placed to test this, besides we already have some integration tests here.

Hmm I didn't remember having those tests. It looks like they aren't run in CI though, and the infrastructure is not setup currently to run them. Do you know how we can get a bitcoin regtest node to run in the github test environment?

I do agree that relying on the lib for these tests is not ideal, but we made a decision a long time ago to use the bitcoind simulator instead of using an actual bitcoin node. Do you recall the reason for that? It was a considerable effort to write the simulator so I feel like there must have been a good reason but honestly I can't recall it right now

gregdhill · 2022-08-01T09:26:39Z

It looks like they aren't run in CI though, and the infrastructure is not setup currently to run them. Do you know how we can get a bitcoin regtest node to run in the github test environment?

Perhaps this was an oversight in the migration from GitLab to GitHub (although it may have actually been Jenkins). @ns212 would be best placed to answer your question though.

I do agree that relying on the lib for these tests is not ideal, but we made a decision a long time ago to use the bitcoind simulator instead of using an actual bitcoin node. Do you recall the reason for that? It was a considerable effort to write the simulator so I feel like there must have been a good reason but honestly I can't recall it right now

I can't recall the exact reason no, but I think the simulator has its advantages - particularly for unit tests. FWIW I'm not going to block this PR on you adding integration tests but we should definitely test this here and even start to pull other complexities out of the lib.

…tc inclusion

gregdhill

The bumping implementation does seem overly complex which makes it hard to follow exactly, but I get the intent. I've left a few comments pertaining to the "always increase" approach which I'm hoping you can provide some more insight on.

bitcoin/src/lib.rs

vault/src/execution.rs

vault/src/vaults.rs

gregdhill · 2022-08-15T14:03:31Z

vault/src/execution.rs

+                .try_filter_map(|x| async move {
+                    match btc_rpc.fee_rate(txid) {
+                        Ok(current_fee) => {
+                            if x >= current_fee {


I am wondering if we should make this configurable, at least if the fee rate increases significantly enough the Vault might prefer to eat the penalty for non-inclusion right?

the Vault might prefer to eat the penalty for non-inclusion right?

I don't understand what you mean here, can you rephrase/elaborate? Are you imagining a scenario where the fees are greater than the sent amount? Keep in mind that when we first creat the tx, we also use the fees that are determined by the oracle's estimate, so I don't think it's bad to always try to use the up-to-date fee rate

Okay so this might be an absurd example, but my point is that we may not want to always blindly increase the fee.

So in reference to the fee budget taken from the oracle when the redeem request is made, let's assume we have the following:

sat/byte is 100

estimated redeem size is 384 bytes

fee budget is 38400 sat or 0.000384 BTC = ~$10

redeem value is 0.1 BTC (total is 0.100384 BTC)

There is no technical limit to the fee rate so let us assume this increases to 3000 sat/byte which the Vault has to cover out-of-pocket:

vault now pays 1152000 sat (0.01152 BTC = ~$275) - maybe less depending on actual size

total sent is now 0.11152 BTC

Now assume the Vault decided not to bump the transaction fees:

slashed 100% value in collateral (e.g. DOT)

punishment fee (10%) of 0.1 BTC also taken = 0.01 BTC (this may also be increased in the future)

total lost is 0.11 BTC - lower net loss than bumping the transaction fee

As @sander2 has just reminded me, once the transaction has been sent the Vault anyway risks losing funds if it doesn't bump to include before expiry.

gregdhill · 2022-08-15T14:04:36Z

vault/src/execution.rs

+                                txid = new_txid;
+                                continue 'outer;
+                            }
+                            Err(Error::BitcoinError(x)) if x.rejected_by_network_rules() => {


Will this catch the case that the Vault can not afford the fee increase?

Hmm that's a good one, probably not. We should add an explicit catch for that I guess

I decided to not abort at all when getting errors. We'll just wait for the original tx

gregdhill · 2022-08-15T14:09:22Z

vault/src/execution.rs

+                    }
+                    Either::Right((Some(Ok((old_fee, new_fee))), continuation)) => {
+                        tracing::debug!("Attempting to bump fee rate from {old_fee} to {new_fee}...");
+                        match self.bump_fee(btc_rpc, txid, new_fee).await {


Is it possible to end up in a sort of "deadlock" if the Vault is always increasing fees, say if we do receive numerous smaller fee estimate bumps on the parachain? (assuming re-submission pushes the new tx to the "back" of the mempool)

I don't see how that'd be possible, no. If bump_fee succeeds, the new tx will end up more to the front of the queue because the miners sort by fee. If it fails there'd be no change

gregdhill · 2022-08-16T12:27:42Z

vault/src/execution.rs

        let fee_rate = self.get_fee_rate(parachain_rpc).await?;

-        tracing::debug!("Using fee_rate = {fee_rate}");
+        tracing::debug!("Using fee_rate = {} sat/vByte", fee_rate.0);


Nit: could implement Display for SatPerVbyte

gregdhill · 2022-08-16T12:28:20Z

vault/src/execution.rs

                        .checked_div(FixedU128::accuracy())
                        .ok_or(Error::ArithmeticUnderflow)
-                        .and_then(|x| x.try_into().map_err(Into::<Error>::into));
+                        .and_then(|x| x.try_into().map(|x| SatPerVbyte(x)).map_err(Into::<Error>::into));


Suggested change

.and_then(|x| x.try_into().map(|x| SatPerVbyte(x)).map_err(Into::<Error>::into));

.and_then(|x| x.try_into().map(SatPerVbyte).map_err(Into::<Error>::into));

gregdhill

LGTM!

sander2 force-pushed the feat/automatic-rbf branch 2 times, most recently from e2b7079 to 4221bbc Compare July 26, 2022 17:03

gregdhill reviewed Jul 27, 2022

View reviewed changes

sander2 force-pushed the feat/automatic-rbf branch from 4221bbc to e656cfd Compare August 11, 2022 14:27

sander2 added 5 commits August 11, 2022 16:46

chore: add to_address function to PartialAddress

95bd544

feat: add all required bitcoin calls to perform RBF bump

441421a

feat: listener for fee_rate changes

6ee8614

feat: bump fees if a new fee estimate is received while waiting for b…

e237879

…tc inclusion

test: add missing trait items to mocks

039281f

sander2 force-pushed the feat/automatic-rbf branch from e656cfd to 221939d Compare August 11, 2022 14:54

test: rbf integration tests using bitcoind

f4cc2d0

sander2 force-pushed the feat/automatic-rbf branch from 221939d to f4cc2d0 Compare August 12, 2022 06:56

gregdhill reviewed Aug 15, 2022

View reviewed changes

sander2 added 3 commits August 15, 2022 19:35

chore: cleanup

e8e8e87

fix: filter for insufficient funds in execution

2f4226c

refactor: introduce explicit SatPerVbyte type

5183d85

gregdhill reviewed Aug 16, 2022

View reviewed changes

sander2 added 2 commits August 16, 2022 19:49

fix: don't try to bump fee if the rate does not strictly increase

510d9d7

fix: make auto-rbf opt-in for now

ab4c31b

gregdhill approved these changes Aug 17, 2022

View reviewed changes

gregdhill merged commit 9c987b2 into interlay:master Aug 17, 2022

gregdhill mentioned this pull request Jun 9, 2023

feat: add advanced rbf guide interlay/interlay.github.io#237

Open

	.and_then(\|x\| x.try_into().map(\|x\| SatPerVbyte(x)).map_err(Into::<Error>::into));
	.and_then(\|x\| x.try_into().map(SatPerVbyte).map_err(Into::<Error>::into));

Conversation

sander2 commented Jul 22, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregdhill commented Jul 27, 2022

Uh oh!

sander2 commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregdhill commented Aug 1, 2022

Uh oh!

gregdhill left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sander2 Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregdhill left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sander2 commented Jul 29, 2022 •

edited

Loading

sander2 Aug 15, 2022 •

edited

Loading