test: add valid tx test with minimum-sized ECDSA signature (8 bytes DER-encoded) #32924

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202507-test-add_minimum-sized_ecdsa_sig_test changing 1 files +4 −0
  1. theStack commented at 6:22 pm on July 8, 2025: contributor

    Currently in our tests, all ECDSA signatures passing verification have sizes of 69 bytes and above (that’s the DER-encoded size, i.e. counted without the sighash flag byte) [1]. This PR adds test coverage for the minimum-sized valid case of 8 bytes, by taking an interesting testnet transaction that I stumbled upon: https://mempool.space/testnet/tx/c6c232a36395fa338da458b86ff1327395a9afc28c5d2daa4273e410089fd433 Note that this is a very obscure construction that only works because the public key used isn’t contained in the locking script, but calculated and provided later at spending time (see https://bitcointalk.org/index.php?topic=1729534.msg17309060#msg17309060 for an explainer), to match the message (sighash) and picked signature. So this doesn’t represent a use-case that really makes sense in practice, but it can still appear in a block (not in mempool though, due to SCRIPT_VERIFY_CONST_SCRIPTCODE), and having test-coverage seems useful.

    Can be tested with same patch below (tests crash with the condition >= 9, but pass with >= 8).

    [1] this can be verified by applying the following patch and running the tests:

     0diff --git a/src/pubkey.cpp b/src/pubkey.cpp
     1index a4ca9a170a..bee0caa603 100644
     2--- a/src/pubkey.cpp
     3+++ b/src/pubkey.cpp
     4@@ -288,7 +288,9 @@ bool CPubKey::Verify(const uint256 &hash, const std::vector<unsigned char>& vchS
     5     /* libsecp256k1's ECDSA verification requires lower-S signatures, which have
     6      * not historically been enforced in Bitcoin, so normalize them first. */
     7     secp256k1_ecdsa_signature_normalize(secp256k1_context_static, &sig, &sig);
     8-    return secp256k1_ecdsa_verify(secp256k1_context_static, &sig, hash.begin(), &pubkey);
     9+    bool ret = secp256k1_ecdsa_verify(secp256k1_context_static, &sig, hash.begin(), &pubkey);
    10+    if (ret) assert(vchSig.size() >= 69);
    11+    return ret;
    12 }
    
  2. test: add valid tx test with minimum-sized ECDSA signature (8 bytes DER-encoded)
    see https://mempool.space/testnet/tx/c6c232a36395fa338da458b86ff1327395a9afc28c5d2daa4273e410089fd433
    and https://bitcointalk.org/index.php?topic=1729534.msg17309060#msg17309060
    5fa81e239a
  3. DrahtBot added the label Tests on Jul 8, 2025
  4. DrahtBot commented at 6:22 pm on July 8, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32924.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK ajtowns, real-or-random, fjahr

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

  5. Sammie05 commented at 3:40 am on July 13, 2025: none

    Thanks for adding coverage for this very specific edge case.

    Using a real testnet transaction is a solid way to ensure the test reflects actual chain data. Also appreciate the clear comments explaining why this minimal 8-byte DER signature can appear and why it matters.

  6. achow101 requested review from ajtowns on Oct 22, 2025
  7. achow101 requested review from fjahr on Oct 22, 2025
  8. achow101 requested review from Eunovo on Oct 22, 2025
  9. achow101 requested review from stratospher on Oct 22, 2025
  10. ajtowns commented at 10:14 pm on October 29, 2025: contributor

    ACK 5fa81e239a39d161a6d5aba7bcc7e1f22a5be777 lgtm

    Presumably it would also be possible to use the same signature on a p2wsh tx (<sig> SWAP CHECKSIG) which would lose the CONST_SCRIPTCODE requirement. Could also presumably just have the script be CHECKSIG and provide both the pubkey and the short signature in either p2sh or p2wsh.

  11. fanquake commented at 10:21 am on October 30, 2025: member
  12. real-or-random approved
  13. real-or-random commented at 11:09 am on October 30, 2025: contributor
  14. ajtowns removed review request from ajtowns on Oct 30, 2025
  15. fjahr commented at 3:43 pm on October 30, 2025: contributor

    tACK 5fa81e239a39d161a6d5aba7bcc7e1f22a5be777

    Reviewed code and verified the behavior with the suggestions in the PR description.

  16. fanquake merged this on Oct 30, 2025
  17. fanquake closed this on Oct 30, 2025

  18. theStack deleted the branch on Oct 30, 2025

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: 2025-12-08 21:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me