Release 2.05.0.2.1 (hotfix: check chain work when processing reorgs) #3152

jcnelson · 2022-05-26T04:13:30Z

Okay, this is now ready for review.

This is a hotfix to the SPV client that addresses the following bugs:

The SPV client now tracks the best chain work for each Bitcoin difficulty adjustment interval it has ever seen. Any attempt to store headers that would lead to a decrease in total chain work will result in an error which will cause the node to re-attempt to download those headers indefinitely until they have at least the best-seen work.
The SPV client will verify that newly-obtained headers after the initial header sync have timestamps that are within 2 hours of the current epoch time.
The SPV client will verify that each Bitcoin header has a timestamp that is strictly greater than the median of the past 11 Bitcoin blocks.
The SPV client will reject a block that has a strictly higher value than the target for the difficulty interval, instead of an equal or higher target.

By far the most work in this PR concerns point #1. To build confidence in the implementation, I added a unit test that is disabled by default, but if set up with the proper environment variables, will download the mainnet Bitcoin block headers and verify that the calculated chain work in each interval is equal to values I manually extracted from bitcoind. In addition, I took PoC bad block headers from the Immunefi bug report and created two unit tests whereby:

A new chain with the bad blocks is discovered and compared to the main chain with the good blocks (this new chain is verified ignored because it has lower total chain work), and
A new chain with the good blocks is discovered and compared to the main chain with the bad blocks (this new chain is verified accepted because it has a higher total chain work).

There are a couple of modifications to BitcoinIndexer::find_bitcoin_reorg() that necessitated changes to Burnchain::sync_with_indexer() that are difficult to test in both unit and integration tests. I am currently in the process of filling this end-to-end test checklist:

A node with this patch is able to resume from existing mainnet chainstate and reach the chain tip
A node with this patch is able to boot from genesis on mainnet and reach the chain tip
A node with this patch is able to resume from existing testnet chainstate and reach the chain tip
A node with this patch is able to boot from genesis on testnet and reach the chain tip

Once these end-to-end tests pass, I think we can make a hotfix release. Once the first end-to-end test passes (resuming from existing mainnet chainstate), I think we can cut a release candidate.

Reminder to anyone reading this: This project participates in Immunefi to pay bounties for bugs such as this one. This bug was reported via Immunefi and is confirmed to exist, as this PR proves.

…ndexer's find_bitcoin_reorg() method to check the original and reorg chain's total work in order to decide whether to move forward with reorg processing.

…BUT, right now the work calculation for a difficulty adjustment interval is getting the wrong answers and I don't know why.

…et, which is what gets used in the difficulty comparison

codecov · 2022-05-26T04:44:43Z

Codecov Report

Merging #3152 (762e007) into master (4641001) will decrease coverage by 1.32%.
The diff coverage is 19.03%.

@@            Coverage Diff             @@
##           master    #3152      +/-   ##
==========================================
- Coverage   83.73%   82.40%   -1.33%     
==========================================
  Files         260      260              
  Lines      201055   203317    +2262     
==========================================
- Hits       168355   167545     -810     
- Misses      32700    35772    +3072

Impacted Files	Coverage Δ
src/burnchains/bitcoin/mod.rs	`38.02% <0.00%> (-1.11%)`	⬇️
src/burnchains/bitcoin/indexer.rs	`33.28% <11.48%> (-50.93%)`	⬇️
src/burnchains/bitcoin/spv.rs	`73.91% <55.68%> (-3.23%)`	⬇️
...-common/src/deps_common/bitcoin/blockdata/block.rs	`86.50% <85.71%> (+9.52%)`	⬆️
src/burnchains/burnchain.rs	`89.87% <87.50%> (-0.26%)`	⬇️
stacks-common/src/util/uint.rs	`90.26% <93.75%> (+2.44%)`	⬆️
src/chainstate/burn/db/sortdb.rs	`95.29% <100.00%> (-0.04%)`	⬇️
src/net/atlas/download.rs	`44.80% <0.00%> (-37.58%)`	⬇️
clarity/src/vm/events.rs	`52.26% <0.00%> (-17.09%)`	⬇️
src/net/atlas/db.rs	`82.45% <0.00%> (-9.36%)`	⬇️
... and 46 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 26bfd5f...762e007. Read the comment docs.

…chainwork into the original SPV database, instead of re-downloading the same headers and hoping for the best. Also, update the mainnet unit test to compare chainwork against known chainwork from bitcoind

… headers

…s the bookkeeping on the SPV DB, so it doesn't have to

…t to download (off-by-one error)

kantai · 2022-05-27T15:33:32Z

stacks-common/src/util/uint.rs

+                    let bytes = self.0[i].to_le_bytes();
+                    for j in 0..bytes.len() {
+                        ret[$n_words * 8 - 1 - (i * 8 + j)] = bytes[j];
+                    }


I think using to_be_bytes() makes this a little easier to follow

Suggested change

let bytes = self.0[i].to_le_bytes();

for j in 0..bytes.len() {

ret[$n_words * 8 - 1 - (i * 8 + j)] = bytes[j];

}

let word_end = $n_words * 8 - (i * 8);

let word_start = word_end - 8;

ret[word_start..word_end]

.copy_from_slice(&self.0[i].to_be_bytes());

kantai · 2022-05-27T15:38:12Z

stacks-common/src/util/uint.rs

@@ -671,4 +734,21 @@ mod tests {
            Uint256([0, 0xDEADBEEFDEADBEEF, 0xDEADBEEFDEADBEEF, 0])
        );
    }
+
+    #[test]
+    pub fn hex_codec() {


The changes in uint need one or two more complex vectors -- this test only covers handling a single word of content.

kantai · 2022-05-27T20:07:38Z

src/burnchains/bitcoin/indexer.rs

+                    // * needs the last difficulty interval of headers (note that the current
+                    // interval is `start_block / BLOCK_DIFFICULTY_CHUNK_SIZE - 1).
+                    // * needs the last interval's chain work calculation
+                    let interval_start_block = start_block / BLOCK_DIFFICULTY_CHUNK_SIZE - 2;


What is interval_start_block? If BLOCK_DIFFICULTY_CHUNK_SIZE is 500, and start_block is 502, this is -2? Should that panic or underflow?

This should be a saturating subtraction.

kantai · 2022-05-27T20:12:05Z

src/burnchains/bitcoin/indexer.rs

+            let reorg_total_work = reorg_spv_client.update_chain_work()?;
+            let orig_total_work = orig_spv_client.get_chain_work()?;
+
+            debug!("Bitcoin headers history is consistent up to {}. Orig chainwork: {}, reorg chainwork: {}", new_tip, orig_total_work, reorg_total_work);


Looks like a good case for k-v syntax

kantai · 2022-05-27T20:13:28Z

src/burnchains/bitcoin/indexer.rs

+                info!(
+                    "New canonical Bitcoin chain found! New tip is {}",
+                    &hdr_reorg.header.bitcoin_hash()
+                );


Does this log line appear on every new bitcoin block, or only on reorgs?

Only on reorgs. This can only get hit if the newly-discovered headers (the "reorg" headers) have more total work than the headers currently in the headers DB (the "orig" headers).

kantai

This looks generally good to me -- I had one question about some of the potentially underflowing math in bitcoin::indexer which I think needs to be addressed.

On top of that, I think this needs to update the CHANGELOG.md with a new release section.

gregorycoppola

Many lines are showing up as "not covered by test" in the code coverage, e.g.:

Is this an artifact of CodeCov or are they really not covered?

jcnelson · 2022-05-29T02:19:10Z

@gregorycoppola I'm not seeing what you're seeing?

…over to the .reorg DB, and use K/V logging. Also, add multi-word test vectors to hex codec for uint256

gregorycoppola · 2022-05-31T13:42:58Z

@jcnelson

I think the linking to CodeCov system is kind of buggy/confusing, because when I click the link it doesn't bring me to the line I started with.

I'm guessing you mean you don't know which lines I'm referring to (but maybe you mean you see the lines but you think they are actually covered).

I think the D3 in the link revers to the file src/burnchains/bitcoin/spv.rs, because it's the third file on the diff list.

I'm attaching some screen shots.

jcnelson · 2022-05-31T16:09:05Z

@gregorycoppola Oh I see why. The new unit tests require you to supply a mainnet headers DB, whose path is indicated by the enviorn BLOCKSTACK_SPV_HEADERS_DB. I'm not sure how to go about adding this to the CI test suite, but in the interest of getting this hotfix released, I'm inclined to address it in a later PR. You can manually run these tests yourself; I have done so many times. @kantai thoughts?

gregorycoppola · 2022-05-31T16:19:40Z

@jcnelson i'm not trying to create busy work in the middle of a hot fix.. just checking what was going on.

if you think it's tested, that's fine.

jcnelson · 2022-05-31T16:23:50Z

@jcnelson i'm not trying to create busy work in the middle of a hot fix.. just checking what was going on.

Ah, sorry, I didn't realize how dismissive I sounded in that comment! I'm glad you brought the code coverage up -- it's important that we do get this covered in CI. I was only trying to get the reviewers' permission to do so in a separate PR :)

gregorycoppola

I'm not totally getting the higher-level picture of what exactly the bug was, why it was a bug, and why this fix is needed.

But, it seems to me like the code is implementing the change described in the description... so I'll leave it to you guys to verify that the change makes sense.

jcnelson · 2022-06-03T02:39:21Z

Okay everyone, both the Foundation and Hiro have confirmed that this node is able to both boot from existing chainstate and spawn from genesis on both mainnet and testnet. If that's sufficient in your eyes for cutting a release, please approve this.

kantai

LGTM!

jcnelson added 5 commits May 26, 2022 00:11

chore: convert immunefi bug report into test vectors and update the i…

74cc416

…ndexer's find_bitcoin_reorg() method to check the original and reorg chain's total work in order to decide whether to move forward with reorg processing.

chore: add InvalidDifficulty variant

04d2275

feat: update the SPV client to track the total chain work over time. …

998b9b7

…BUT, right now the work calculation for a difficulty adjustment interval is getting the wrong answers and I don't know why.

chore: unused mut

11f6d7e

feat: add static method to convert a bitcoin target to a compact targ…

9425a39

…et, which is what gets used in the difficulty comparison

jcnelson requested a review from kantai May 26, 2022 04:13

chore: add uint256 codec to/from hex strings

e8b2cec

jcnelson added 8 commits May 26, 2022 13:55

feat: have find_bitcoin_reorg() actually merge the reorg headers and …

c6ac355

…chainwork into the original SPV database, instead of re-downloading the same headers and hoping for the best. Also, update the mainnet unit test to compare chainwork against known chainwork from bitcoind

chore: s/InvalidDifficulty/InvalidChainWork/g

fcbc660

fix: use the correct chainwork calculation by summing over individual…

4721f67

… headers

fix: update sync_with_indexer() to assume that find_chain_reorg() doe…

6c9221a

…s the bookkeeping on the SPV DB, so it doesn't have to

chore: add big-endian code for uint256, which is easier to read

7670d02

fix: test that the SPV chain tip is within 2 hours of now

437381e

refactor: better name

debf40c

fix: prime the .reorg DB with the parent of the first header we expec…

1cc98b5

…t to download (off-by-one error)

jcnelson marked this pull request as ready for review May 27, 2022 01:47

jcnelson requested review from pavitthrap and gregorycoppola May 27, 2022 01:47

kantai reviewed May 27, 2022

View reviewed changes

gregorycoppola reviewed May 27, 2022

View reviewed changes

jcnelson requested review from kantai and gregorycoppola May 29, 2022 02:19

fix: use .saturating_sub() when determining the header range to copy …

9f510fd

…over to the .reorg DB, and use K/V logging. Also, add multi-word test vectors to hex codec for uint256

jcnelson added 2 commits May 28, 2022 22:45

fix: compile-time error

e1319a9

chore: cargo fmt

5b326f9

chore: add changelog for SPV chain work fix

762e007

gregorycoppola approved these changes May 31, 2022

View reviewed changes

jcnelson changed the title ~~hotfix: check chain work when processing reorgs~~ Release 2.05.0.2.1 (hotfix: check chain work when processing reorgs) Jun 3, 2022

kantai approved these changes Jun 3, 2022

View reviewed changes

jcnelson merged commit 38aa968 into master Jun 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release 2.05.0.2.1 (hotfix: check chain work when processing reorgs) #3152

Release 2.05.0.2.1 (hotfix: check chain work when processing reorgs) #3152

jcnelson commented May 26, 2022 •

edited

codecov bot commented May 26, 2022 •

edited

kantai May 27, 2022 •

edited

jcnelson May 29, 2022

kantai May 27, 2022

jcnelson May 29, 2022

kantai May 27, 2022 •

edited

jcnelson May 29, 2022

kantai May 27, 2022

jcnelson May 29, 2022

kantai May 27, 2022

jcnelson May 29, 2022

kantai left a comment

gregorycoppola left a comment

jcnelson commented May 29, 2022

gregorycoppola commented May 31, 2022

jcnelson commented May 31, 2022

gregorycoppola commented May 31, 2022

jcnelson commented May 31, 2022

gregorycoppola left a comment

jcnelson commented Jun 3, 2022

kantai left a comment

-                    let bytes = self.0[i].to_le_bytes();
-                    for j in 0..bytes.len() {
-                        ret[$n_words * 8 - 1 - (i * 8 + j)] = bytes[j];
-                    }
+                    let word_end = $n_words * 8 - (i * 8);
+                    let word_start = word_end - 8;
+                    ret[word_start..word_end]
+                          .copy_from_slice(&self.0[i].to_be_bytes());

Release 2.05.0.2.1 (hotfix: check chain work when processing reorgs) #3152

Release 2.05.0.2.1 (hotfix: check chain work when processing reorgs) #3152

Conversation

jcnelson commented May 26, 2022 • edited

codecov bot commented May 26, 2022 • edited

Codecov Report

kantai May 27, 2022 • edited

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 May 27, 2022 • edited

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

gregorycoppola left a comment

Choose a reason for hiding this comment

jcnelson commented May 29, 2022

gregorycoppola commented May 31, 2022

jcnelson commented May 31, 2022

gregorycoppola commented May 31, 2022

jcnelson commented May 31, 2022

gregorycoppola left a comment

Choose a reason for hiding this comment

jcnelson commented Jun 3, 2022

kantai left a comment

Choose a reason for hiding this comment

jcnelson commented May 26, 2022 •

edited

codecov bot commented May 26, 2022 •

edited

kantai May 27, 2022 •

edited

kantai May 27, 2022 •

edited