bitcoinconsensus: invalid flags error should be set to bitcoinconsensus_err #13655

pull afk11 wants to merge 1 commits into bitcoin:master from afk11:bitcoinconsensus-invalid-flags-squashed changing 2 files +143 −1

afk11 commented at 12:38 PM on July 13, 2018: contributor

A check was added to the bitcoinconsensus verify_script codepath to ensure that callers only used exposed interpreter flags. I think this error should be written to bitcoinconsensus_err* err and not returned by verify_script?

I modified the check so it indicates the error using *err like the others, and added tests covering the error codes.
afk11 renamed this:
~~bitcoinconsensus: invalid flags should be set to bitcoinconsensus_err~~
bitcoinconsensus: invalid flags error should be set to bitcoinconsensus_err
on Jul 13, 2018

in src/test/script_tests.cpp:1636 in 5506b7f14d outdated

1631 | +    stream << spendTx;
1632 | +
1633 | +    bitcoinconsensus_error err;
1634 | +    int result = bitcoinconsensus_verify_script(scriptPubKey.data(), scriptPubKey.size(), (const unsigned char*)&stream[0], stream.size(), nIn, libconsensus_flags, &err);
1635 | +    BOOST_CHECK(result == 0);
1636 | +    BOOST_CHECK(err == bitcoinconsensus_ERR_INVALID_FLAGS);

afk11 commented at 1:40 PM on July 13, 2018:

Previous behavior:

result == bitcoinconsensus_ERR_INVALID_FLAGS (5)
err is untouched

Empact commented at 4:34 PM on July 13, 2018: member

nit: You could use BOOST_CHECK_EQUAL throughout. If you get a compilation failure, see below for an example of how to make bitcoinconsensus_ERR_* supported for output.

https://github.com/bitcoin/bitcoin/blob/2ea7eb62b21affeb56fcea7433cfff9e5bc4742c/src/test/test_bitcoin.cpp#L42-L46
Empact commented at 6:13 PM on July 13, 2018: member

utACK 5506b7f
afk11 commented at 7:02 PM on July 13, 2018: contributor

Thanks, BOOST_CHECK_EQUAL worked without further changes, pushing that now
Empact commented at 8:17 PM on July 13, 2018: member

re-utACK 0d1cc23 please squash
bitcoinconsensus: invalid flags should be set to bitcoinconsensus_error type, add test cases covering bitcoinconsensus error codes 417b6c1d29
afk11 force-pushed on Jul 13, 2018
afk11 commented at 9:13 PM on July 13, 2018: contributor

squashed!
sipa commented at 9:47 PM on July 13, 2018: member

utACK 417b6c1d2990ffc78c029442e027797d724a101f
fanquake added the label Utils/log/libs on Jul 15, 2018
laanwj commented at 2:56 PM on July 18, 2018: member

utACK 417b6c1d2990ffc78c029442e027797d724a101f
laanwj merged this on Jul 18, 2018
laanwj closed this on Jul 18, 2018
laanwj referenced this in commit 585db54dd1 on Jul 18, 2018
afk11 deleted the branch on Jul 18, 2018
luke-jr referenced this in commit 1cdbea7f74 on Jul 29, 2018
laanwj referenced this in commit 6518bcd56c on Jul 30, 2018
HashUnlimited referenced this in commit 5dd2101459 on Jan 11, 2019
PastaPastaPasta referenced this in commit c71344eda3 on Apr 12, 2020
Fabcien referenced this in commit 685164613e on Jan 6, 2021
UdjinM6 referenced this in commit bad069dc08 on Jun 29, 2021
UdjinM6 referenced this in commit b9a8cfca83 on Jun 29, 2021
UdjinM6 referenced this in commit e0d2b848f6 on Jul 1, 2021
UdjinM6 referenced this in commit 805ae63323 on Jul 2, 2021
UdjinM6 referenced this in commit 056a2b356b on Jul 2, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#13797 [0.16] bitcoinconsensus: invalid flags should be set to bitcoinconsensus_error type, add test cases covering bitcoinconsensus error codes