Simplify IsFinalTx logic. #19518

1. 
   roconnor-blockstream commented at 5:17 pm on July 14, 2020: contributor

   There is no need to explicitly check tx.nLockTime == 0. If tx.nLockTime is 0, then it will be compared with nBlockHeight, which will only fail if nBlockHeight is 0. nBlockHeight is only 0 for the Genesis block (see https://github.com/bitcoin/bitcoin/blob/f4de89edfa8be4501534fec0c662c650a4ce7ef2/src/validation.cpp#L3544) The Genesis block is fixed and its sequence number is final. Therefore the IsFinalTx will still return true, even if it is called on the Genesis block.

2. Simplify IsFinalTx logic.

   There is no need to explicitly check tx.nLockTime == 0.
   If tx.nLockTime is 0, then it will be compared with nBlockHeight, which will only fail if nBlockHeight is 0.
   nBlockHeight is only 0 for the Genesis block (see https://github.com/bitcoin/bitcoin/blob/f4de89edfa8be4501534fec0c662c650a4ce7ef2/src/validation.cpp#L3544)
   The Genesis block is fixed and its sequence number is final.
   Therefore the IsFinalTx will still return true, even if it is called on the Genesis block.

   b97ee24178
3. 
   JeremyRubin commented at 5:42 pm on July 14, 2020: contributor

   This change appears to implement identical logic except in the cases noted.

   My preference would be if we’re going to be cleaning up this code, to clean up the gross ternary expression to be easier to read rather than a (modified/validated genesis block excluded) mostly redundant check.

   Is there a motivating reason for this change other than code cleanup?

4. 
   roconnor-blockstream commented at 5:51 pm on July 14, 2020: contributor

   My personal motivation is that I was trying to interpret the locktime rules in order to work with them. I replicated the logic as it was written here originally, but upon reflection it seemed unnecessarily complex. I cleaned up the logic in my own work and thought I might as well suggest the same clean up for Bitcoin Core while I was on the topic.
5. 
   JeremyRubin commented at 6:01 pm on July 14, 2020: contributor

   Might be interesting to check out a previous PR I opened which cleaned up consensus logic for similar reasons. #15969 (comment), as it discusses a large number of contributors perceived priorities around simplifying consensus logic.
6. laanwj added the label Validation on Jul 15, 2020
7. 
   sdaftuar commented at 11:40 pm on July 28, 2020: member

   I’m a Concept NACK on changes like this.

   While this patch seems correct to me, I think that changing consensus code should have a pretty high bar – if there is no discernible benefit to the change (such as performance, or allowing for development of new features), then I’d prefer that we (at most) document our understanding of the logic in cases like this, rather than change it.

   If our documentation ends up being wrong, no harm done; if instead an incorrect change to consensus code slips through, we risk splitting the network. As a project, I don’t think it makes sense to spend the effort that would be required to make the latter a sufficiently low-risk choice when compared with the former. Our efforts are better spent on adding features and fixing bugs.

8. 
   MarcoFalke commented at 4:29 am on July 29, 2020: member

   Too controversial for a simple fix like this. Closing for now.
9. MarcoFalke closed this on Jul 29, 2020

   ---

10. MarcoFalke added the label Refactoring on Jul 29, 2020
11. DrahtBot locked this on Feb 15, 2022

Contributors
roconnor-blockstream JeremyRubin sdaftuar MarcoFalke

Labels
Refactoring Validation