test: Add assumeutxo test for wrong hash #28647

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2310-test-au- changing 1 files +14 −8

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