validation: add const for minimum witness commitment size #18780

pull fanquake wants to merge 2 commits into bitcoin:master from fanquake:minimum_witness_size changing 3 files +45 −2
  1. fanquake commented at 5:50 am on April 27, 2020: member

    https://github.com/bitcoin/bitcoin/commit/16101de5f33be494019a3f81755e204d00c22347: Per BIP 141, the witness commitment structure is at least 38 bytes, OP_RETURN (0x6a) + 36 (0x24) + 4 byte header (0xaa21a9ed) + 32 byte SHA256 hash. It can be longer, however any additional data has no consensus meaning.

    https://github.com/bitcoin/bitcoin/commit/54f8c48d6ac973024df35c4db038791b7958a51d: As per BIP 141, if there is more than 1 pubkey that matches the witness commitment structure, the one with the highest output index should be chosen. This adds a sanity check that we are doing that, which will fail if anyone tries to “optimize” GetWitnessCommitmentIndex() by returning early.

  2. fanquake added the label Validation on Apr 27, 2020
  3. fanquake requested review from sipa on Apr 27, 2020
  4. in src/validation.cpp:3396 in 54f8c48d6a outdated 
    3392+            && vout.scriptPubKey[0] == OP_RETURN
3393+            && vout.scriptPubKey[1] == 0x24
3394+            && vout.scriptPubKey[2] == 0xaa
3395+            && vout.scriptPubKey[3] == 0x21
3396+            && vout.scriptPubKey[4] == 0xa9
3397+            && vout.scriptPubKey[5] == 0xed) {
    MarcoFalke commented at 11:45 am on April 27, 2020:

    style-nit: If you reformat this, might as well make it conform to clang-format:

     0diff --git a/src/validation.cpp b/src/validation.cpp
 1index 28538cfbbd..5411e45d7d 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -3387,13 +3387,13 @@ int GetWitnessCommitmentIndex(const CBlock& block)
 5     if (!block.vtx.empty()) {
 6         for (size_t o = 0; o < block.vtx[0]->vout.size(); o++) {
 7             CTxOut vout = block.vtx[0]->vout[o];
 8-            if (vout.scriptPubKey.size() >= MINIMUM_WITNESS_COMMITMENT
 9-            && vout.scriptPubKey[0] == OP_RETURN
10-            && vout.scriptPubKey[1] == 0x24
11-            && vout.scriptPubKey[2] == 0xaa
12-            && vout.scriptPubKey[3] == 0x21
13-            && vout.scriptPubKey[4] == 0xa9
14-            && vout.scriptPubKey[5] == 0xed) {
15+            if (vout.scriptPubKey.size() >= MINIMUM_WITNESS_COMMITMENT &&
16+                vout.scriptPubKey[0] == OP_RETURN &&
17+                vout.scriptPubKey[1] == 0x24 &&
18+                vout.scriptPubKey[2] == 0xaa &&
19+                vout.scriptPubKey[3] == 0x21 &&
20+                vout.scriptPubKey[4] == 0xa9 &&
21+                vout.scriptPubKey[5] == 0xed) {
22                 commitpos = o;
23             }
24         }
    fanquake commented at 0:30 am on April 28, 2020:
    Done.
  5. MarcoFalke approved
  6. MarcoFalke commented at 11:48 am on April 27, 2020: member
    Concept ACK. Thanks for adding the test.
  7. DrahtBot commented at 12:30 pm on April 27, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18471 (qa: Test shared validation interface by promag)
    • #18267 (BIP-325: Signet [consensus] by kallewoof)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  8. fanquake force-pushed on Apr 28, 2020
  9. in src/test/validation_block_tests.cpp:364 in a9db322e01 outdated 
    359+
360+    CMutableTransaction txCoinbase(*pblock->vtx[0]);
361+    txCoinbase.vout.resize(3);
362+    txCoinbase.vout[0] = witness;
363+    txCoinbase.vout[1] = witness;
364+    txCoinbase.vout[2] = witness;
    ajtowns commented at 10:26 am on April 28, 2020:
    Could consider adding vout[3] as an invalid commitment (too short, or not starting with OP_RETURN?), and vout[2] having scriptPubKey.resize(MIN_WIT_COM + 1) ?
    fanquake commented at 3:26 am on April 29, 2020:
    Thanks, I’ve added an additional vout with OP_VERIFY and modified [2] to be 38 + 1.
  10. ajtowns approved
  11. ajtowns commented at 11:51 am on April 28, 2020: member
    ACK a9db322e017281811e5039c80cd3114c5025ee27
  12. in src/validation.cpp:3389 in a9db322e01 outdated 
    3385@@ -3386,7 +3386,14 @@ int GetWitnessCommitmentIndex(const CBlock& block)
3386     int commitpos = -1;
3387     if (!block.vtx.empty()) {
3388         for (size_t o = 0; o < block.vtx[0]->vout.size(); o++) {
3389-            if (block.vtx[0]->vout[o].scriptPubKey.size() >= 38 && block.vtx[0]->vout[o].scriptPubKey[0] == OP_RETURN && block.vtx[0]->vout[o].scriptPubKey[1] == 0x24 && block.vtx[0]->vout[o].scriptPubKey[2] == 0xaa && block.vtx[0]->vout[o].scriptPubKey[3] == 0x21 && block.vtx[0]->vout[o].scriptPubKey[4] == 0xa9 && block.vtx[0]->vout[o].scriptPubKey[5] == 0xed) {
3390+            CTxOut vout = block.vtx[0]->vout[o];
    MarcoFalke commented at 12:16 pm on April 28, 2020: 
    0            const CTxOut& vout = block.vtx[0]->vout[o];

    for read-only access this can be a const ref?

  13. in src/validation.h:96 in a9db322e01 outdated 
    91@@ -92,6 +92,8 @@ static const unsigned int DEFAULT_CHECKLEVEL = 3;
92 // one 128MB block file + added 15% undo data = 147MB greater for a total of 545MB
93 // Setting the target to >= 550 MiB will make it likely we can respect the target.
94 static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
95+/** Minimum size of a witness commitment structure. Defined in BIP 141. **/
96+static const unsigned int MINIMUM_WITNESS_COMMITMENT = 38;
    MarcoFalke commented at 12:20 pm on April 28, 2020: 
    0static constexpr size_t MINIMUM_WITNESS_COMMITMENT{38};

    The type where this is passed in our out seems to be size_t. See https://en.cppreference.com/w/cpp/container/vector/size and https://en.cppreference.com/w/cpp/container/vector/resize. So might as well use that type here.

  14. in src/test/validation_block_tests.cpp:349 in a9db322e01 outdated 
    344+BOOST_AUTO_TEST_CASE(witness_commitment_index)
345+{
346+    CScript pubKey;
347+    pubKey << 1 << OP_TRUE;
348+    auto ptemplate = BlockAssembler(*m_node.mempool, Params()).CreateNewBlock(pubKey);
349+    auto pblock = std::make_shared<CBlock>(ptemplate->block);
    MarcoFalke commented at 12:22 pm on April 28, 2020:

    Doesn’t need to be a shared pointer, since you are not passing it around?

    0    CBlock block = ptemplate->block;
  15. MarcoFalke approved
  16. MarcoFalke commented at 12:23 pm on April 28, 2020: member
    ACK, only style-nits. Feel free to ignore if you don’t have to push for other reasons.
  17. validation: Add minimum witness commitment size constant 
    Per BIP 141, the witness commitment structure is atleast 38 bytes,
OP_RETURN (0x6a) + 36 (0x24) + 4 byte header (0xaa21a9ed) + 32 byte
SHA256 hash. It can be longer, however any additional data has no
consensus meaning.
    06442549f8
  18. test: add test for witness commitment index 
    As per BIP 141, if there is more than 1 pubkey that matches the witness
commitment structure, the one with the highest output index should be
chosen. This adds a sanity check that we are doing that, which will fail
if anyone trys to "optimise" GetWitnessCommitmentIndex() be returning
early.
    692f8307fc
  19. fanquake force-pushed on Apr 29, 2020
  20. laanwj commented at 6:35 am on April 29, 2020: member
    ACK 692f8307fc1449299b90182e7d79efb81a55d7ab
  21. MarcoFalke commented at 2:18 pm on April 29, 2020: member

    ACK 692f8307fc1449299b90182e7d79efb81a55d7ab 🌵

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 692f8307fc1449299b90182e7d79efb81a55d7ab 🌵
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUichAwAuEMVEDQF3EgTUf50uRHePnrKLa4HtuTzm7DoVwZYx/7h2WHmnOaFUxNr
 8WN00g4AnUP98j4bKJIhiy+1MOwBjJKgkG9vk45MUfqy2ULU2YlP1mJyfaQOIL+3g
 9pWvKRHIITbYF9+KM0NlF4eTdg+92uhm6Amt3VH6NA410B5gtDHaB4f3uP5814qkl
10v/e2jjxTeJQEMqBVXzxd3IsdV/t5kaNX7FQyZcWQr8FPFQtgM03KrA3rX+/0jiLW
11Fc+EUxdj1e7ub8d2zyls3fl+DevmNwIDD22tGZoG4+mPvXJ75vlS6CaxDaBFEaeY
12sZCQ6ptrhafb25KSwj3gQeTNtsc/GSab+jDMvazp72PgRN4ZkUqM6+E0o6rg9mEt
13kohPV9BX85hDblH72pPsd7DAslpUB84/iCUAMLKxvuGTd2uqaLyUsuAs915undbm
14t/g5stIrN63anYVBFW9kMb6uA/vdczn6WBO97QfFvCeMPc6ERQ0raRQbF+4COtd6
15c59QjzLf
16=sydW
17-----END PGP SIGNATURE-----

    Timestamp of file with hash 1559b3b98f5ec3d13a900bc1b3f63ad4a5ef128875774ede7f164cc133dabb52 -

  22. jonatack commented at 4:12 pm on April 29, 2020: member
    Code review ACK 692f830
  23. jnewbery commented at 4:21 pm on April 29, 2020: member
    utACK 692f8307fc1449299b90182e7d79efb81a55d7ab
  24. ajtowns commented at 2:32 am on April 30, 2020: member
    ACK 692f8307fc1449299b90182e7d79efb81a55d7ab
  25. kallewoof commented at 7:21 am on April 30, 2020: member
    Code review ACK
  26. fanquake merged this on Apr 30, 2020
  27. fanquake closed this on Apr 30, 2020
  28. fanquake deleted the branch on May 1, 2020
  29. sidhujag referenced this in commit 57e2d71631 on May 2, 2020
  30. DrahtBot locked this on Feb 15, 2022


fanquake MarcoFalke DrahtBot ajtowns laanwj jonatack jnewbery kallewoof


sipa

Labels
Validation

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me