test: check tx is final when there is no locktime #33030

pull brunoerg wants to merge 1 commits into bitcoin:master from brunoerg:2025-07-test-isfinaltx-nlocktime-0 changing 1 files +4 −0

brunoerg commented at 6:41 PM on July 21, 2025: contributor

According to https://corecheck.dev/mutation/src/consensus/tx_verify.cpp, there is no proper test for the tx.nLockTime == 0 check in the IsFinalTx function, which is understandable, since this check will only be useful for a specific case where the nBlockHeight (block height) is zero. Otherwise, the following check if ((int64_t)tx.nLockTime < ((int64_t)tx.nLockTime < LOCKTIME_THRESHOLD ? (int64_t)nBlockHeight : nBlockTime)) would catch any of it. This PR adds a test case for it.
DrahtBot added the label Tests on Jul 21, 2025
DrahtBot commented at 6:41 PM on July 21, 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/33030.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK maflcko, darosior, enirox001, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
darosior approved
darosior commented at 6:55 PM on July 21, 2025: member

Makes sense. utACK 7180b82420c0f303140a93ca635f5ac806bea77d. Could add a comment explaining this to the test as it may not be immediately obvious to a reader.
test: IsFinalTx returns true when there is no locktime 065e42976a
brunoerg force-pushed on Jul 22, 2025
brunoerg commented at 1:04 PM on July 22, 2025: contributor

Makes sense. utACK 7180b82. Could add a comment explaining this to the test as it may not be immediately obvious to a reader.

Thank you, I just added a comment for this.
maflcko commented at 1:17 PM on July 22, 2025: member

lgtm ACK 065e42976a70738770af256da810ddc1316a4496
DrahtBot requested review from darosior on Jul 22, 2025

Type	Reviewers
ACK	maflcko, darosior, enirox001, achow101

in src/test/miner_tests.cpp:526 in 065e42976a

 522 | @@ -523,6 +523,10 @@ void MinerTestingSetup::TestBasicMining(const CScript& scriptPubKey, const std::
 523 |      BOOST_CHECK(TestSequenceLocks(CTransaction{tx}, tx_mempool)); // Sequence locks pass
 524 |      BOOST_CHECK(IsFinalTx(CTransaction(tx), m_node.chainman->ActiveChain().Tip()->nHeight + 2, m_node.chainman->ActiveChain().Tip()->GetMedianTimePast())); // Locktime passes on 2nd block
 525 |  
 526 | +    // ensure tx is final for a specific case where there is no locktime and block height is zero

darosior commented at 1:22 PM on July 22, 2025:

nit, don't retouch this for me, but my point with the comment wasn't to describe the line below (anyone can just read block height is 0 and locktime is 0) but to describe why them both being 0 is a special case worth exercising, as you did in OP.

darosior approved
darosior commented at 1:22 PM on July 22, 2025: member

utACK 065e42976a70738770af256da810ddc1316a4496
enirox001 commented at 3:36 PM on July 22, 2025: contributor

lgtm

ACK 065e429: Valuable test case that explicitly demonstrates IsFinalTx behavior when nLockTime is 0
achow101 commented at 5:49 PM on July 22, 2025: member

ACK 065e42976a70738770af256da810ddc1316a4496
achow101 merged this on Jul 22, 2025
achow101 closed this on Jul 22, 2025
brunoerg deleted the branch on Jul 22, 2025

Contributors

Labels

test: check tx is final when there is no locktime #33030

Code Coverage & Benchmarks

Reviews