Feat/bitcoin wire format rigidity #3126

jcnelson · 2022-05-05T16:58:56Z

This PR addresses #2094 by making it so that when we process a Stacks block, we search the 6 prior burnchain blocks for unprocessed TransferStx and StackStx transactions and try to apply them in this block. This tracking is handled by mapping each Stacks block's index block hash to the list of burnchain transaction IDs it handled. Then, to process a new Stacks block, we simply do the following:

find all burnchain operations in the past 6 burnchain blocks that are ancestors of the burnchain block that selected this Stacks block
find all burnchain operations processed in the past 6 Stacks blocks that are ancestors of the pending Stacks block (which covers at least 6 burnchain blocks in the past)
remove all burnchain operations in (2) from the list in (1)
process the remaining operations from (3) in blockchain order

…given burnchain header hash, which we will use to pull out a window of previously-mined on-burnchain stacks transactions

…ks to find all on-burnchain blocks that haven't yet been processed in this Stacks fork. Process them all, instead of the ones that just happen to land in this stacks block's burnchain block.

…given the index block hash of a Stacks block.

… Stacks block (keyed by index block hash), so we can quickly load them up when searching for *un*processed burnchain-hosted transactions when processing a new Stacks block.

…ocks

… can be mined and processed in Stacks blocks even if the burnchain block that contains them does not have a sortition with a stacks block

codecov · 2022-05-05T17:31:04Z

Codecov Report

Merging #3126 (43d8fb5) into next (d8f7628) will increase coverage by 0.37%.
The diff coverage is 97.19%.

@@            Coverage Diff             @@
##             next    #3126      +/-   ##
==========================================
+ Coverage   83.63%   84.00%   +0.37%     
==========================================
  Files         268      268              
  Lines      209655   210798    +1143     
==========================================
+ Hits       175338   177077    +1739     
+ Misses      34317    33721     -596

Impacted Files	Coverage Δ
src/core/mod.rs	`99.50% <ø> (+22.66%)`	⬆️
src/chainstate/burn/db/sortdb.rs	`95.21% <88.67%> (-0.02%)`	⬇️
testnet/stacks-node/src/tests/epoch_21.rs	`96.68% <96.64%> (+96.68%)`	⬆️
src/chainstate/stacks/db/mod.rs	`87.39% <97.36%> (+0.46%)`	⬆️
src/chainstate/stacks/db/blocks.rs	`89.51% <98.96%> (+0.50%)`	⬆️
src/chainstate/stacks/db/accounts.rs	`92.24% <100.00%> (+0.01%)`	⬆️
src/chainstate/stacks/db/headers.rs	`88.50% <100.00%> (+0.70%)`	⬆️
src/net/mod.rs	`72.16% <100.00%> (+0.11%)`	⬆️
testnet/stacks-node/src/tests/neon_integrations.rs	`81.46% <100.00%> (+0.01%)`	⬆️
clarity/src/vm/database/sqlite.rs	`78.47% <0.00%> (-1.39%)`	⬇️
... and 39 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73df355...43d8fb5. Read the comment docs.

gregorycoppola

clarification: Why/when are TransferSTX and StackSTX written to the burnchain? I actually didn't realize that was happening.

pavitthrap

lgtm

pavitthrap · 2022-05-06T21:18:09Z

src/chainstate/stacks/db/headers.rs

@@ -331,4 +331,33 @@ impl StacksChainState {
            .map_err(|e| Error::DBError(db_error::SqliteError(e)))?
            .is_some())
    }
+
+    /// Load up the past N ancestors' index block hashes of a given block, *including* the given


past N ancestors'... not including

The comment is correct. The given argument index_block_hash is included in the returned list of StacksBlockIds.

pavitthrap · 2022-05-10T15:44:54Z

src/chainstate/stacks/db/blocks.rs

@@ -11080,6 +11233,557 @@ pub mod test {
        );
    }

+    fn make_transfer_op(


Add cfg[test]?

It's already in a mod test { ... } block decorated with #[cfg(test)].

pavitthrap · 2022-05-10T16:00:10Z

src/chainstate/stacks/db/blocks.rs

+        peer_config.initial_balances = vec![(addr.to_account_principal(), initial_balance)];
+        peer_config.epochs = Some(StacksEpoch::unit_test_2_1(0));
+
+        let recv_addr =


pavitthrap · 2022-05-10T16:00:20Z

src/chainstate/stacks/db/blocks.rs

+        peer_config.initial_balances = vec![(addr.to_account_principal(), initial_balance)];
+        peer_config.epochs = Some(StacksEpoch::unit_test_2_1(0));
+
+        let recv_addr =


also unused

pavitthrap · 2022-05-10T16:00:46Z

src/chainstate/stacks/db/blocks.rs

+        transfer_op
+    }
+
+    #[test]


short description of test would be good, ditto for the next test

pavitthrap · 2022-05-10T16:14:27Z

testnet/stacks-node/src/tests/epoch_21.rs

+    // mine it
+    next_block_and_wait(&mut btc_regtest_controller, &blocks_processed);
+
+    // let's fire off our transfer op that will not land in a sortition pre-2.1


this comment could be worded more clearly

jcnelson · 2022-05-12T00:57:14Z

clarification: Why/when are TransferSTX and StackSTX written to the burnchain? I actually didn't realize that was happening.

Per SIP-007, these operations exist in order to ensure that Stacks miners will pay a high price to attempt to censor them. If these operations did not exist, then Stacks miners could simply choose to ignore these transactions in the Stacks mempool. But because a Stacks block is only valid if it includes the burnchain transactions in the same burnchain that sortitioned it, the cost of censoring these transactions is the cost of foregoing a Stacks block reward (which is much higher than the cost of the BTC transaction).

The reason we're worried about miner censorship is because there are scenarios where miners can be incentivized to block them. Miners are not only potential stackers, whose income from PoX is influenced to other stackers' stacking, but also are able to take bribes from other stackers in order to keep the stacking minimum down (and thus accrue more BTC). By making censorship very expensive for miners, we make it costly for miners to deviate from honest mining behavior in this matter.

This PR not only makes on-burnchain transfer and stacking operations less brittle and more likely to get confirmed by the network, but also significantly increases the cost of censorship by doing so. Miners would need to forego 6x the block reward to block an on-burnchain stack-stx or transfer-stx operation.

…consideration window in our tests

jcnelson · 2022-06-06T18:02:08Z

@kantai @gregorycoppola Need at least one of you to review please :)

kantai · 2022-06-06T20:23:55Z

src/chainstate/stacks/db/blocks.rs

@@ -4599,6 +4601,8 @@ impl StacksChainState {
                                   "txid" => %txid,
                                   "burn_block" => %burn_header_hash,
                                   "contract_call_ecode" => %resp.data);
+                        } else {
+                            debug!("Processed StackStx burn op for {} uSTX for {} cycles starting at {} from {} to pay out to {}", stacked_ustx, num_cycles, block_height, &sender, &reward_addr);


All of those fields should be k-v entries:

Suggested change

debug!("Processed StackStx burn op for {} uSTX for {} cycles starting at {} from {} to pay out to {}", stacked_ustx, num_cycles, block_height, &sender, &reward_addr);

debug!("Processed StackStx burnchain op"; "amount_ustx" => stacked_ustx, "num_cycles" => num_cycles, "burn_block_height" => block_height, "sender" => %sender, "reward_addr" => %reward_addr);

Would be useful to add the txid as well

kantai · 2022-06-06T20:25:00Z

src/chainstate/stacks/db/blocks.rs

+                                debug!(
+                                    "Processed TransferStx {} uSTX from {} to {}",
+                                    transfered_ustx, &sender, &recipient
+                                );


Same comment as above: this should be a k-v log, with txid.

kantai · 2022-06-06T20:29:48Z

src/chainstate/stacks/db/blocks.rs

+                if processed_burnchain_txids.contains(&stacking_op.txid) {
+                    continue;
+                }
+                all_stacking_burn_ops.push(stacking_op);


The control flow is simpler if the if statement checks for exclusion:

Suggested change

if processed_burnchain_txids.contains(&stacking_op.txid) {

continue;

}

all_stacking_burn_ops.push(stacking_op);

if !processed_burnchain_txids.contains(&stacking_op.txid) {

all_stacking_burn_ops.push(stacking_op);

}

kantai · 2022-06-06T20:31:40Z

src/chainstate/stacks/db/blocks.rs

+    fn get_stacking_and_transfer_burn_ops_v210<'b>(
+        chainstate_tx: &'b mut ChainstateTx,


I don't think the lifetime generic is necessary here: rust can almost always figure out the lifetimes for references that don't land in the return type.

kantai · 2022-06-06T20:31:51Z

src/chainstate/stacks/db/blocks.rs

+    fn get_stacking_and_transfer_burn_ops<'b>(
+        chainstate_tx: &'b mut ChainstateTx,
+        parent_index_hash: &StacksBlockId,
+        sortdb_conn: &Connection,
+        burn_tip: &BurnchainHeaderHash,
+        burn_tip_height: u64,
+    ) -> Result<(Vec<StackStxOp>, Vec<TransferStxOp>), Error> {


I don't think the lifetime generic is necessary here: rust can almost always figure out the lifetimes for references that don't land in the return type.

kantai

This all looks great to me -- just superficial comments!

…m/stacks-network/stacks-blockchain into feat/bitcoin-wire-format-rigidity

…et to the first_block_height before spawning p2p

jcnelson added 9 commits May 5, 2022 12:50

chore: add 2.1 tests to CI

40568d7

feat: add a way to query the ancestor burnchain header hashes from a …

11a9487

…given burnchain header hash, which we will use to pull out a window of previously-mined on-burnchain stacks transactions

chore: api sync

46e7250

feat: in Stacks 2.1, search the burnchain back a given window of bloc…

9ef7915

…ks to find all on-burnchain blocks that haven't yet been processed in this Stacks fork. Process them all, instead of the ones that just happen to land in this stacks block's burnchain block.

feat: add a method to get the index block hashes of ancestor blocks, …

56d878a

…given the index block hash of a Stacks block.

feat: track all burnchain-hosted transactions we've processed in each…

2a40381

… Stacks block (keyed by index block hash), so we can quickly load them up when searching for *un*processed burnchain-hosted transactions when processing a new Stacks block.

feat: set the burnchain tx search window to 6 sortitions

f65910d

chore: fix the test suite so we can have sortitions with no stacks bl…

a9d19b9

…ocks

chore: add an integration test to verify that on-burnchain operations…

d4fcc0b

… can be mined and processed in Stacks blocks even if the burnchain block that contains them does not have a sortition with a stacks block

jcnelson requested review from kantai, pavitthrap and gregorycoppola May 5, 2022 16:58

jcnelson added the stacks-2.1 label May 5, 2022

jcnelson added 2 commits May 5, 2022 12:59

chore: expose get_balance()

102e1f3

Merge branch 'next' into feat/bitcoin-wire-format-rigidity

24535e5

chore: cargo fmt

ec932d0

gregorycoppola reviewed May 9, 2022

View reviewed changes

jcnelson added 2 commits May 10, 2022 02:41

Merge branch 'next' into feat/bitcoin-wire-format-rigidity

ea4deb5

fix: merge artifact: PoxConstants has a new constructor

065f5cf

pavitthrap approved these changes May 10, 2022

View reviewed changes

jcnelson added 2 commits May 11, 2022 21:16

chore: better comments about what's happening with the new burnchain …

4ee0f6c

…consideration window in our tests

Merge branch 'next' into feat/bitcoin-wire-format-rigidity

732f69c

jcnelson requested a review from gregorycoppola June 6, 2022 15:27

kantai reviewed Jun 6, 2022

View reviewed changes

kantai approved these changes Jun 6, 2022

View reviewed changes

jcnelson added 2 commits June 6, 2022 21:34

chore: use k/v logging, omit unneeded lifetimes

02f5180

Merge branch 'feat/bitcoin-wire-format-rigidity' of https://github.co…

43d8fb5

…m/stacks-network/stacks-blockchain into feat/bitcoin-wire-format-rigidity

jcnelson merged commit 1c7db10 into next Jun 7, 2022

kantai added a commit that referenced this pull request Aug 1, 2022

fix: adress startup lock (#3126) by only waiting for coordinator to g…

1ee596e

…et to the first_block_height before spawning p2p

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/bitcoin wire format rigidity #3126

Feat/bitcoin wire format rigidity #3126

jcnelson commented May 5, 2022 •

edited

Loading

codecov bot commented May 5, 2022 •

edited

Loading

gregorycoppola left a comment

pavitthrap left a comment

pavitthrap May 6, 2022

jcnelson May 12, 2022

pavitthrap May 10, 2022

jcnelson May 12, 2022

pavitthrap May 10, 2022

jcnelson May 12, 2022

pavitthrap May 10, 2022

jcnelson May 12, 2022

pavitthrap May 10, 2022

jcnelson May 12, 2022

pavitthrap May 10, 2022

jcnelson May 12, 2022

jcnelson commented May 12, 2022

jcnelson commented Jun 6, 2022

kantai Jun 6, 2022

kantai Jun 6, 2022

kantai Jun 6, 2022

kantai Jun 6, 2022

kantai Jun 6, 2022

kantai Jun 6, 2022

kantai left a comment

	debug!("Processed StackStx burn op for {} uSTX for {} cycles starting at {} from {} to pay out to {}", stacked_ustx, num_cycles, block_height, &sender, &reward_addr);
	debug!("Processed StackStx burnchain op"; "amount_ustx" => stacked_ustx, "num_cycles" => num_cycles, "burn_block_height" => block_height, "sender" => %sender, "reward_addr" => %reward_addr);

		fn get_stacking_and_transfer_burn_ops_v210<'b>(
		chainstate_tx: &'b mut ChainstateTx,

Feat/bitcoin wire format rigidity #3126

Feat/bitcoin wire format rigidity #3126

Conversation

jcnelson commented May 5, 2022 • edited Loading

codecov bot commented May 5, 2022 • edited Loading

Codecov Report

gregorycoppola left a comment

Choose a reason for hiding this comment

pavitthrap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcnelson commented May 12, 2022

jcnelson commented Jun 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kantai left a comment

Choose a reason for hiding this comment

jcnelson commented May 5, 2022 •

edited

Loading

codecov bot commented May 5, 2022 •

edited

Loading