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 a test to catch the empty merkle root bug #562

Closed
ebuchman opened this issue Sep 11, 2020 · 3 comments · Fixed by #588
Closed

Add a test to catch the empty merkle root bug #562

ebuchman opened this issue Sep 11, 2020 · 3 comments · Fixed by #588
Assignees
Labels
Milestone

Comments

@ebuchman
Copy link
Member

Not totally clear why this wasn't being caught by integration tests but we should have some simple integration test that would catch #498. I guess there's nothing currently checking the header field hashes or even the hash of the header itself based on the block data? It's a bit of block validation functionality which we don't really have yet but we should start with enough to catch this so at least our Merkle tree is being tested in real context of being in a block.

The bug was fixed in #557 and has a unit test but no block-level test.

@ebuchman ebuchman added the tests label Sep 11, 2020
@ebuchman ebuchman added this to the v0.17.0 milestone Sep 11, 2020
@Shivani912
Copy link
Contributor

Jumping on this now. Just to be sure (because we use the term "integration test" interchangeably with "conformance test" sometimes), you mean tests like here? @ebuchman

@ebuchman
Copy link
Member Author

Right, so we could just fetch a block from the RPC and based on the data in the block, recompute the data_hash field of the header ourselves (ie. merkle root of the transactions) and check that it matches. Since we're not sending txs, blocks will be empty, so we can check that we compute the right merkle root when there are no txs. That should be sufficient to capture this.

Proper follow up would include a few different scenarios, like blocks with with 0, 1, 2, 5 transactions or something, but we'd have to do more work to create them. And ultimately we'd want tests that check all fields in the header against the block and other state, but we don't need to worry about that for now.

@Shivani912
Copy link
Contributor

Perfect! Thanks for explaining @ebuchman :)

Shivani912 added a commit that referenced this issue Sep 29, 2020
ebuchman pushed a commit that referenced this issue Sep 29, 2020
* add empty merkle root test (#562)

* remove unwanted import

* cargo fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants