test: Fix SegwitV0SignatureMsg nLockTime signedness #29400
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
This can be tested by passing a transaction with a large locktime and then observing the error: |
| @@ -747,7 +747,7 @@ def SegwitV0SignatureMsg(script, txTo, inIdx, hashtype, amount): | |||
| ss += struct.pack("<q", amount) | |||
| ss += struct.pack("<I", txTo.vin[inIdx].nSequence) | |||
| ss += ser_uint256(hashOutputs) | |||
| ss += struct.pack("<i", txTo.nLockTime) | |||
| ss += txTo.nLockTime.to_bytes(4, "little") | |||
There was a problem hiding this comment.
Could we keep the format?
| ss += txTo.nLockTime.to_bytes(4, "little") | |
| struct.pack("<I", txTo.nLockTime) |
In the instructions you've mentioned it's reproducible - can you add a test that fails before this change, but passes after?
There was a problem hiding this comment.
Yes, you can try this with the following diff:
diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py
index d316c4b602..708e4d7e8f 100755
--- a/test/functional/p2p_segwit.py
+++ b/test/functional/p2p_segwit.py
@@ -1663,6 +1663,7 @@ class SegWitTest(BitcoinTestFramework):
pubkeyhash = hash160(pubkey)
script_pkh = key_to_p2wpkh_script(pubkey)
tx = CTransaction()
+ tx.nLockTime = 0xffffffff
tx.vin.append(CTxIn(COutPoint(temp_utxos[0].sha256, temp_utxos[0].n), b""))
tx.vout.append(CTxOut(temp_utxos[0].nValue, script_pkh))
tx.wit.vtxinwit.append(CTxInWitness())
To add a test, you may have to disable the locktime by setting the sequence number to the right value as well.
I am happy to push a commit that adds a test, if someone writes one.
There was a problem hiding this comment.
can you add a test that fails before this change, but passes after?
Wouldn't that mean that we added a test that tests the test-framework?
There was a problem hiding this comment.
can you add a test that fails before this change, but passes after?
Wouldn't that mean that we added a test that tests the test-framework?
Yes, it should be possible to write an internal python unit test to check the behavior of SegwitV0SignatureMsg. An alternative would be to extend a python functional test to cover more values of nLockTime and compare the SegwitV0SignatureMsg behavior against Bitcoin Core via the RPC interface.
|
Tested ACK fab1572. |
|
Tested ACK fab1572 |
|
ACK fab1572 |
fa52e13 test: Remove struct.pack from almost all places (MarcoFalke) fa826db scripted-diff: test: Use int.to_bytes over struct packing (MarcoFalke) faf2a97 test: Use int.to_bytes over struct packing (MarcoFalke) faf3cd6 test: Normalize struct.pack format (MarcoFalke) Pull request description: `struct.pack` has many issues: * The format string consists of characters that may be confusing and may need to be looked up in the documentation, as opposed to using easy to understand self-documenting code. This lead to many test bugs, which weren't hit, which is fine, but still confusing. Ref: #29400, #29399, #29363, fa3886b, ... Fix all issues by using the built-in `int` helpers `to_bytes` via a scripted diff. Review notes: * For `struct.pack` and `int.to_bytes` the error behavior is the same, although the error messages are not identical. * Two `struct.pack` remain. One for float serialization in a C++ code comment, and one for native serialization. ACKs for top commit: achow101: ACK fa52e13 rkrux: tACK [fa52e13](fa52e13) theStack: Code-review ACK fa52e13 Tree-SHA512: ee80d935b68ae43d1654b047e84ceb80abbd20306df35cca848b3f7874634b518560ddcbc7e714e2e7a19241e153dee64556dc4701287ae811e26e4f8c57fe3e
It is unsigned in Bitcoin Core, so the tests should match it:
bitcoin/src/script/interpreter.cpp
Line 1611 in 5b8990a
The bug was introduced when the code was written in 330b0f3.
(Lowercase
imeans signed, see https://docs.python.org/3/library/struct.html#format-characters)