Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Use RBF for recovering from fast blocks #2383

Merged
merged 56 commits into from
Feb 2, 2021

Conversation

lgalabru
Copy link
Contributor

@lgalabru lgalabru commented Jan 26, 2021

This PR is superseding #2363 and addressing a few issues, with one refactoring;

  • It allows a miner to recover from a "fast block", by RBF-ing submitted block commits trying to target a "flashed" block.
  • It's preventing submission of multiple block commit / block.
  • Improving the RBF logic, by dynamically computing the size of the transactions (the values from config are still used but now only for initial estimation).

I was able to test the miner against a puppet chain and see the RBF logic successfully kicking in with multiple consecutive flash blocks (3 consecutive RBF, 1 final mined transaction), I should also be able to write some unit tests.

How to test:

  1. start bitcoind
$ cat helium.conf
chain=regtest
disablewallet=0
txindex=1
server=1
rpcuser=helium-node
rpcpassword=secret%

$ bitcoind -conf=helium.conf
  1. start puppet chain
$ cd testnet/puppet-chain
$ DYNAMIC_GENESIS_TIMESTAMP=1 cargo run local-leader.toml.default
  1. start a stacks-node

in core/mod.rs update the constants

pub const BITCOIN_REGTEST_FIRST_BLOCK_HEIGHT: u64 = 300;
pub const BITCOIN_REGTEST_FIRST_BLOCK_TIMESTAMP: u32 = 1611158538;
pub const BITCOIN_REGTEST_FIRST_BLOCK_HASH: &str =
    "47922cbadd8158ea999b6be9a31dd69daf58b33dd5be2d3d2d528274d5dd8ea2";

I'd also recommend enforce usage of a sampled genesis.

$ cd testnet/stacks-node
$ STACKS_LOG_PP=1 cargo run start --config=./conf/local-leader-conf.toml

The puppet chain will basically be messing around with bitcoind between block 307 and block 312, generating flash blocks and buffering transactions coming from the stacks-node.
When everything goes back to normal and inspecting block 313, we can see that all the block commit that happened during 307 and 312 have been RBF, and we only have one clean block commit.

@lgalabru lgalabru self-assigned this Jan 26, 2021
@lgalabru lgalabru force-pushed the feat/rewrite-bitcoin-controller branch from 14d49ff to ecda499 Compare January 26, 2021 14:51
@lgalabru lgalabru force-pushed the feat/rewrite-bitcoin-controller branch from 20856ed to d06be96 Compare January 26, 2021 19:49
}

// Did a re-org occurred since we fetched our UTXOs
if let Err(e) = burnchain_db.get_burnchain_block(&ongoing_op.utxos.bhh) {
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 think this is ever going to fail. We don't delete data from BurnchainDB, so a sibling Bitcoin block's block hash (and all its associated ops) would continue to be accessible through get_burnchain_block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I added this condition to have a green light from the test bitcoind_forking_test. Do you think we should leave that condition, or rewrite that test?

Copy link
Member

@jcnelson jcnelson Feb 1, 2021

Choose a reason for hiding this comment

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

I think the error path here can arise and is definitely worth handling, but I think the check you want here is to verify whether or not ongoing_op.utxos.bhh is still on the canonical burnchain fork (note that this case can flip-flop -- the burn header hash can be on the canonical fork, then off the canonical fork, and then back on it). You can tell by either looking at the SPV headers, or by looking at the sortition DB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed with b03495b

@lgalabru lgalabru force-pushed the feat/rewrite-bitcoin-controller branch from 1007387 to 4d576c9 Compare January 29, 2021 22:33
@jcnelson jcnelson mentioned this pull request Feb 1, 2021
@jcnelson jcnelson self-requested a review February 2, 2021 17:44
@lgalabru lgalabru merged commit 16b15b7 into feat/puppet-chain Feb 2, 2021
@lgalabru lgalabru deleted the feat/rewrite-bitcoin-controller branch February 2, 2021 20:11
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 24, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants