test: Add assumeutxo test for wrong hash

maflcko commented at 10:19 am on October 13, 2023: member

Also:

Update test TODOs
Fix off-by-4 typo in test, remove struct import

DrahtBot commented at 10:19 am on October 13, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fjahr, theStack, pablomartin4btc, ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label Tests on Oct 13, 2023

theStack commented at 10:48 am on October 13, 2023: contributor

Concept ACK

Nice. Tried to work on this as well recently and thought it’s not possible without implementing a VARINT-parser (for the Coin de/serialization), being not aware that the outpoint comes first 😅

theStack approved

theStack commented at 5:11 pm on October 13, 2023: contributor

Code-review ACK fa91fc737dc88899fa7452941327546ec3f73972

maflcko added this to the milestone 26.0 on Oct 16, 2023

pablomartin4btc approved

pablomartin4btc commented at 8:36 pm on October 16, 2023: member

cr ACK fa91fc737dc88899fa7452941327546ec3f73972

in test/functional/feature_assumeutxo.py:20 in fa91fc737d outdated

16@@ -17,8 +17,7 @@
17 
18 Interesting test cases could be loading an assumeutxo snapshot file with:
19 
20-- TODO: An invalid hash
21-- TODO: Valid hash but invalid snapshot file (bad coin height or truncated file or
22+- TODO: Valid hash but invalid snapshot file (bad coin height or

fjahr commented at 8:24 am on October 17, 2023:

Isn’t the bad coin height test also already covered with the test where you made the fix?

maflcko commented at 8:29 am on October 17, 2023:

Yeah, I wonder how one could test a “valid” hash but invalid snapshot file. I guess one would have to add a hash of the invalid file to the source code.

Though, this comment presumably refers to https://maflcko.github.io/b-c-cov/total.coverage/src/validation.cpp.gcov.html#5398 which is still uncovered.

fjahr commented at 9:02 am on October 17, 2023:

I wonder how one could test a “valid” hash but invalid snapshot file

I would just generally understand this as taking a valid snapshot file and altering the content so that it still serializes correctly but results in a different hash. The way I understand it “valid hash” here means that the hash in the metadata of the file matches the hash in the chainparams.

I guess one would have to add a hash of the invalid file to the source code.

That implies that the attacker is able to give the user a build that is not a release, no? (or we have made a huge mistake when adding the hash) If the attackers are able to do that they could do anything so it doesn’t seem feasible to cover such a scenario in our tests.

maflcko commented at 9:15 am on October 17, 2023:

Right. Though, in that case, it could make sense to transform the unreachable/untestable code, if there is any, into an assert fail, or use the Assume fail.

ryanofsky commented at 3:26 pm on October 17, 2023:

Yeah, I wonder how one could test a “valid” hash but invalid snapshot file. I guess one would have to add a hash of the invalid file to the source code.

Yes, my suggestion was to add an invalid regtest hash to the source code so we can verify that assumeutxo code does what it is supposed to do when an invalid hash is added.

And I don’t like the idea of making the executable abort when an invalid hash is detected, instead of returning a normal error message, because that would make it more difficult than it needs to be to write a test covering all the error cases, and also make error handling internally more fragile and less straightforward.

This is just to explain my earlier suggestion though. I see these hashes as external inputs that should be validated like other inputs, even they happen to be hard coded to prevent usage errors. But if you want to treat these hashes as normal source code that should be able to trigger crashes in other parts of the source code, that is also reasonable.

fjahr commented at 8:36 am on October 17, 2023: contributor

utACK fa91fc737dc88899fa7452941327546ec3f73972

DrahtBot added the label Needs rebase on Oct 17, 2023

test: Add assumeutxo test for wrong hash fa68571566

maflcko force-pushed on Oct 17, 2023

DrahtBot removed the label Needs rebase on Oct 17, 2023

fjahr commented at 3:04 pm on October 17, 2023: contributor

utACK fa685715663117955e9bb795cbf79ddbd3dfed19

DrahtBot requested review from theStack on Oct 17, 2023

DrahtBot requested review from pablomartin4btc on Oct 17, 2023

DrahtBot removed review request from pablomartin4btc on Oct 17, 2023

DrahtBot requested review from pablomartin4btc on Oct 17, 2023

theStack approved

theStack commented at 3:13 pm on October 17, 2023: contributor

Code-review re-ACK fa685715663117955e9bb795cbf79ddbd3dfed19

DrahtBot removed review request from pablomartin4btc on Oct 17, 2023

DrahtBot requested review from pablomartin4btc on Oct 17, 2023

pablomartin4btc commented at 3:15 pm on October 17, 2023: member

re ACK fa685715663117955e9bb795cbf79ddbd3dfed19

ryanofsky approved

ryanofsky commented at 4:04 pm on October 17, 2023: contributor

Code review ACK fa685715663117955e9bb795cbf79ddbd3dfed19

ryanofsky merged this on Oct 17, 2023

ryanofsky closed this on Oct 17, 2023

maflcko deleted the branch on Oct 17, 2023

Frank-GER referenced this in commit b5f8054aef on Oct 21, 2023

bitcoin locked this on Oct 16, 2024

test: Add assumeutxo test for wrong hash #28647

Code Coverage

Reviews

Conflicts