It is unsigned in Bitcoin Core, so the tests should match it:
The bug was introduced when the code was written in 330b0f31ee5719d94f9e52dfc83c5d82168241f9.
(Lowercase i means signed, see https://docs.python.org/3/library/struct.html#format-characters)
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | epiccurious, Eunovo, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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:
struct.error: 'i' format requires -2147483648 <= number <= 2147483647
746 | @@ -747,7 +747,7 @@ def SegwitV0SignatureMsg(script, txTo, inIdx, hashtype, amount): 747 | ss += struct.pack("<q", amount) 748 | ss += struct.pack("<I", txTo.vin[inIdx].nSequence) 749 | ss += ser_uint256(hashOutputs) 750 | - ss += struct.pack("<i", txTo.nLockTime) 751 | + ss += txTo.nLockTime.to_bytes(4, "little")
Could we keep the format?
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?
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.
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?
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 fab15723b0518acbb1015e64df47dcac0187e92f.
Tested ACK https://github.com/bitcoin/bitcoin/pull/29400/commits/fab15723b0518acbb1015e64df47dcac0187e92f
Reproduced the error by setting nLockTime to 2147483648
ACK fab15723b0518acbb1015e64df47dcac0187e92f