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

pull jtimon wants to merge 1 commits into bitcoin:master from jtimon:consensus_finaltx changing 1 files +9 −3

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.
laanwj added the label Improvement on Apr 29, 2015
in src/main.cpp:None 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.
jonasschnelli commented at 6:09 PM on May 9, 2015: contributor

Tested ACK. nit: see above.
jtimon force-pushed on May 16, 2015
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).
paveljanik commented at 12:51 PM on May 27, 2015: contributor

utACK

You do not like BOOST_FOREACH? ;-)
Consensus: Refactor: Separate CheckFinalTx from main::IsFinalTx fc176a1716
jtimon force-pushed on May 27, 2015
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...
laanwj commented at 2:01 PM on May 27, 2015: member

utACK, this is contained in #6183
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.
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.
jtimon closed this on Jun 2, 2015
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.
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)
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#6051 MOVEONLY: Consensus: Move most of consensus functions (pre-block)