Consensus: Refactor: Separate CheckFinalTx from main::IsFinalTx #6063

1. 
   jtimon commented at 12:59 pm on April 25, 2015: contributor

   A simple refactor as preparation for moving consensus to code for transaction validation. The consensus version of IsFinalTx() cannot use chainActive.Height() or GetAdjustedTime()

   This is part of #6051 but can be merged independently.

2. laanwj added the label Improvement on Apr 29, 2015
3. in src/main.cpp: in dd06e9efd7 outdated

   643@@ -644,6 +644,7 @@ bool IsStandardTx(const CTransaction& tx, string& reason)
   644     return true;
   645 }
   646 
   647+bool CheckFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime);
   

   ---

   ---

   

   jonasschnelli commented at 5:42 pm on May 9, 2015:

   nit: To avoid this declaration, would it not be possible to move the implementation of CheckFinalTx() (L661) above isFinalTx() (above L648)? IIRC we don’t have function declarations in main.cpp.

   ---

   

   jtimon commented at 12:37 pm on May 10, 2015:

   This declaration will be necessary later anyway when the function is moved in https://github.com/jtimon/bitcoin/commit/6769c02ca34303648de74f42c7dfd0cb1aaa41da#diff-cbe22f30d7e480617350ef6ceca97d0cR65 So it’s a temporary thing, and, yes, we have function declarations in main.cpp, for example, these two: https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L81 The reason to make it in this way is to make the diff smaller.

   ---

   

   jonasschnelli commented at 12:45 pm on May 10, 2015:

   Okay. Fair enought.
4. 
   jonasschnelli commented at 6:09 pm on May 9, 2015: contributor

   Tested ACK. nit: see above.
5. jtimon force-pushed on May 16, 2015
6. 
   jtimon commented at 7:29 pm on May 16, 2015: contributor

   Properly rebased (I missed one recent change in IsFinalTx, the tx.nLockTime == 0 check).
7. 
   paveljanik commented at 12:51 pm on May 27, 2015: contributor

   utACK

   You do not like BOOST_FOREACH? ;-)

8. Consensus: Refactor: Separate CheckFinalTx from main::IsFinalTx fc176a1716
9. jtimon force-pushed on May 27, 2015
10. 
    jtimon commented at 1:38 pm on May 27, 2015: contributor

    Updated without touching the foreach loop as discussed on IRC. Now it’s really really trivial…
11. 
    laanwj commented at 2:01 pm on May 27, 2015: member

    utACK, this is contained in #6183
12. 
    jtimon commented at 2:05 pm on May 27, 2015: contributor

    Well, not exactly, the names are changed in #6183 and the consensus-unfriendly one doesn’t take any parameters besides the transaction.
13. 
    jtimon commented at 11:08 am on June 2, 2015: contributor

    Closing after #6183 has been merged. Btw, @petertodd I think my last suggestion there (not using the activechain global from the new function CheckFinalTx) was a very reasonable nit, I’m a little bit dissapointed that you ignored it.
14. jtimon closed this on Jun 2, 2015

    ---

15. 
    laanwj commented at 11:56 am on June 2, 2015: member

    @jtimon Feel free to fix that nit anyway - but we just had to move ahead to get the bug fix in.
16. 
    petertodd commented at 11:59 am on June 2, 2015: contributor

    Yup, that’s my thoughts here too.

    However give me a week or so and I’ll do up that median time patch design so we can fix this exactly once. On “vacation” right now. :)

    On 2 June 2015 13:57:04 CEST, “Wladimir J. van der Laan” notifications@github.com wrote:

    @jtimon Feel free to fix that nit anyway - but we just had to move ahead to get the bug fix in.

    ---

    Reply to this email directly or view it on GitHub: #6063 (comment)

17. MarcoFalke locked this on Sep 8, 2021

Contributors
jtimon jonasschnelli paveljanik laanwj petertodd

Labels
Refactoring