Fix off-by-one error w/ nLockTime in the wallet

petertodd commented at 5:34 am on May 25, 2015: contributor

Previously due to an off-by-one error the wallet ignored nLockTime-by-height transactions that would be valid in the next block even though they are accepted into the mempool. The transactions wouldn’t show up until confirmed, nor would they be included in the unconfirmed balance. Similar to the mempool behavior fix in 665bdd3b, the wallet code was calling IsFinalTx() directly without taking into account the fact that doing so tells you if the transaction could have been mined in the current block, rather than the next block.

Instead of changing the wallet code, this commit simply changes the semantics of IsFinalTx() when the block height isn’t provided to use the height of the next block. The resulting semantics return true if the transaction in question could included in the next block, significantly less confusing and more useful than the previous semantics.

petertodd force-pushed on May 25, 2015

laanwj added the label Bug on May 26, 2015

laanwj commented at 6:13 am on May 26, 2015: member

This changes the semantics of a function that is used in consensus.

However the only uses of IsFinalTx with default arguments (before this change) are in the wallet and tests so I think it is ok.

Though now that we’re changing this function anyway, I’d prefer splitting up IsFinalTx into:

A function IsFinalTx that takes explicit nBlockHeight nBlockTime arguments. This is a consensus function independent of program state.
A function IsFinalTxNow that calls IsFinalTx with the default arguments (chainActive.Height() + 1 and GetAdjustedTime()), to be used by the wallet and such.

luke-jr commented at 6:35 am on May 26, 2015: member

@laanwj +1

Changing the behaviour of a function without renaming it or otherwise breaking old code using it, is asking for trouble.

laanwj commented at 8:58 am on May 27, 2015: member

See @jtimon’s #6063, it sort of does what I’m describing above, apart from renaming the function.

Fix off-by-one error w/ nLockTime in the wallet

Previously due to an off-by-one error the wallet ignored
nLockTime-by-height transactions that would be valid in the next block
even though they are accepted into the mempool. The transactions
wouldn't show up until confirmed, nor would they be included in the
unconfirmed balance. Similar to the mempool behavior fix in 665bdd3b,
the wallet code was calling IsFinalTx() directly without taking into
account the fact that doing so tells you if the transaction could have
been mined in the *current* block, rather than the next block.

To fix this we strip IsFinalTx() of non-consensus-critical
functionality, removing the default arguments, and add CheckFinalTx() to
check if a transaction will be final in the next block.

28bf06236d

petertodd force-pushed on May 27, 2015

petertodd commented at 10:02 am on May 27, 2015: contributor

@laanwj Split into two functions.

In doing so I also had to add part #6177

Any thoughts on what should happen at https://github.com/bitcoin/bitcoin/blob/28bf06236d3b385e95fe26a7a742395b30efd6ee/src/test/miner_tests.cpp#L251?

jtimon commented at 12:13 pm on May 27, 2015: contributor

Any reason not to just rebase on top of #6063 ? That way the function that keeps the old name does what it used to do while the new one has the different name (which will also be more consistent with other consensus functions and will avoid creating unnecessary conflicts with other open PRs [see #6051 ] and other old branches based on them).

Any thoughts on what should happen at https://github.com/bitcoin/bitcoin/blob/28bf06236d3b385e95fe26a7a742395b30efd6ee/src/test/miner_tests.cpp#L251 ?

I believe testing the new method would be good. This is just another example of how globals make testing more difficult.

in src/test/miner_tests.cpp: in 28bf06236d

247@@ -248,8 +248,10 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
248     chainActive.Tip()->nHeight++;
249     SetMockTime(chainActive.Tip()->GetMedianTimePast()+2);
250 
251-    BOOST_CHECK(IsFinalTx(tx, chainActive.Tip()->nHeight + 1));
252-    BOOST_CHECK(IsFinalTx(tx2));
253+    // FIXME: we should *actually* create a new block so the following test
254+    //        works; CheckFinalTx() isn't fooled by monkey-patching nHeight.
255+    //BOOST_CHECK(CheckFinalTx(tx));

laanwj commented at 1:08 pm on May 27, 2015:

Could just use IsFinalTx here with manual arguments instead of CheckFinalTx for these tests?

petertodd commented at 8:48 pm on May 27, 2015:

You could, but it’d create more code to update if the logic of CheckFinalTx() changes for some reason. Remember that this is really a “test-the-test” line of code, not what the test is actually trying to test.

in src/miner.cpp: in 28bf06236d

136@@ -137,6 +137,7 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
137         LOCK2(cs_main, mempool.cs);
138         CBlockIndex* pindexPrev = chainActive.Tip();
139         const int nHeight = pindexPrev->nHeight + 1;
140+        pblock->nTime = GetAdjustedTime();

laanwj commented at 1:12 pm on May 27, 2015:

Curious that this wasn’t set yet, anyhow it makes sense to do it

laanwj commented at 1:21 pm on May 27, 2015: member

Looks good to me now. @jtimon The reason for renaming the function is outlined in my last post, as well as @luke-jr’s reply - as this pull changes the functionality of the function, so to be sure that all caller sites are ‘aware’ of this it makes sense to rename it. Apart from that I have no opinion on how the functions should be called.

in src/main.cpp: in 28bf06236d

657@@ -658,14 +658,8 @@ bool IsStandardTx(const CTransaction& tx, string& reason)
658 
659 bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime)
660 {
661-    AssertLockHeld(cs_main);
662-    // Time based nLockTime implemented in 0.1.6
663     if (tx.nLockTime == 0)
664         return true;
665-    if (nBlockHeight == 0)

jtimon commented at 2:09 pm on May 27, 2015:

Are you sure none of the current callers of IsFinalTx pass nBlockHeight or nBlockTime = 0 ? Seriously, it seems much safer to keep this function as it is and call the “new”/consensus-friendly one from that one. If you’re breaking the old one, why keep to of them at all? The only reason I maintain the old one in #6063 is to make sure it keeps being functionally identical. If you can’t warranty that, I would just use the consensus-friendly version everywhere.

petertodd commented at 8:51 pm on May 27, 2015:

I am quite sure of that, particularly since the callers that would do that are all consensus-critical code, and this could lead to a consensus split. Edit: maybe that comes off the wrong way… I mean, because it could lead to a consensus split, I spent a good hour or so examining every call of IsFinalTx() to be sure it’d never be called that way.

Fortunately the only block for which nBlockHeight=0 or nBlockTime=0 is legal is the genesis block so we don’t actually have that bug in practice. Additionally if nBlockHeight/nBlockTime are set to zero the function still behaves as you’d expect it to in that circumstance.

jtimon commented at 6:19 am on May 28, 2015:

I am quite sure of that, particularly since the callers that would do that are all consensus-critical code, and this could lead to a consensus split. Edit: maybe that comes off the wrong way… I mean, because it could lead to a consensus split, I spent a good hour or so examining every call of IsFinalTx() to be sure it’d never be called that way.

Then why maintain a global-state-dependent function at all? If we’re going to review all the calls and change all those lines, let’s just have the stateless version of it. And at that point, you’re only changing one function, shouldn’t the criterion “if you change functionality, change the name of the function” precisely apply here?

Fortunately the only block for which nBlockHeight=0 or nBlockTime=0 is legal is the genesis block so we don’t actually have that bug in practice.

Well, the genesis block shouldn’t be tested since it’s correct by definition (is really the first consensus rule), so that’s fine.

Additionally if nBlockHeight/nBlockTime are set to zero the function still behaves as you’d expect it to in that circumstance.

No, before the null values were replaced with values from the global state, now they don’t. They clearly function differently (even if you’re taking care of it not mattering by adapting things on the caller, the function itself is NOT functionally equivalent to the previous stateful one).

petertodd commented at 11:17 am on May 28, 2015:

Then why maintain a global-state-dependent function at all?

Huh? The global state-dependent function is for the convenience of callers, who just need to know if the tx will be final by the time the next block comes.

If we’re going to review all the calls and change all those lines, let’s just have the stateless version of it.

Having only the stateless IsFinalTx() as an option results in most of those calls having useless - and probably soon to be obsolete - Tip()->nHeight, GetAdjustedTime() boilerplate.

And at that point, you’re only changing one function, shouldn’t the criterion “if you change functionality, change the name of the function” precisely apply here?

The functionality of mixing up a consensus-critical and non-consensus-critical function in one is simply broken.

No, before the null values were replaced with values from the global state, now they don’t. They clearly function differently (even if you’re taking care of it not mattering by adapting things on the caller, the function itself is NOT functionally equivalent to the previous stateful one).

Again, the previous design of IsFinalTx() is simply broken and shouldn’t have happened; from the point of view of the important stuff - the consensus-critical code - the functionality hasn’t changed changed. Fortunately we never had a situation where that broken design bit us in the ass so in spite of it we didn’t end up with a forking bug; if the functionality had changed from the point of view of consensus-critical code it’d be a guaranteed forking bug because it’s comparing nLockTime to state that is not the same on every node.

jtimon commented at 2:21 pm on May 27, 2015: contributor

@laanwj Well, a caller like IsFinalTx(tx, 0, 0) won’t work as expected. The new function could be the consensus-friendly one, as in #6063. That would also make my life easier, although I guess saving developers’ time is not that strong of an argument… Keeping IsFinalTx functionally identical (before maybe deprecating it) is a stronger argument. You can also create a third one dependent on the globals and without parameters (like CheckFinalTransaction here) and probably call it IsFinalTx just like the other global-dependent one.

petertodd commented at 8:55 pm on May 27, 2015: contributor

Personally I prefer the terminology “IsFinalTx()” for the “no-context” consensus-critical check, and “CheckFinalTx()” for the context-dependent non-consensus-critical check.

jtimon commented at 6:50 am on May 28, 2015: contributor

Personally I prefer the terminology “IsFinalTx()” for the “no-context” consensus-critical check, and “CheckFinalTx()” for the context-dependent non-consensus-critical check.

As said, Check is being used by other consensus functions (stateless) and IsFinalTx was global-state dependent. For consistency, it would be better to have the consensus version start with Check. But since this may be considered bikesheding, let me explain what the options I see. Let’s imagine this in smaller commits that have been squashed and consider stopping at each point.

Fix the bug without touching IsFinalTx, simply by appropriately adapting the arguments in the pertinent calls (like the one in miner). We could simply stop here (or at any point).
Separate a stateless (consensus-friendly) version of the function, like in #6063 .
Every call to the old function that passes both arguments and passes them as non-null values, can be safely replaced with a call to the new function, great. You can go further and only leave the old calls that were using the default null values (triggering the replacement with the global state).
Now we can safely remove the optional parameters from the old function, since nobody is using them (the calls that use the parameters now call to the new statelss function).
Maybe we wanted to make the fix at this point instead of doing it first, since now you can fix it inside the stateless function and presumably less calls will be affected.
Assuming we had chosen IsFinalTx for the old global-state-dependent function and CheckFinalTx for the new stateless version (as in #6063 ), we can now switch the names to gratuitously increase the total diff, make the stateless version name less consistent with other consensus functions names and complicate the rebase of branches that were already separating those functions (like #6051 and branches based on it) or that were touching changing IsFinalTx (like https://github.com/maaku/bitcoin/commits/seqno-cltv-csv).

Let’s please stop before 6. Let’s forget about naming and making me and possibly other people work more. I propose to use a cleaner git history as the main criterion. My approach is better because it will produce a smaller total diff (since some calls that were using the default values can remain untouched).

petertodd commented at 11:43 am on May 28, 2015: contributor

Something you’re probably not aware of re: CheckFinalTx() is that I’ll be submitting another pull-req once this is merged to change CheckFinalTx() to use GetMedianTime() rather than GetAdjustedTime(). This is important for a number of security related reasons. (most discussed before on #bitcoin-wizards) After that I’ll be proposing a soft-fork enforcing this within blocks themselves.

My thinking re: not renaming IsFinalTx() is taking that into account, because IsFinalTx() in that case would be relegated to purely consensus-critical function deep in the bowels of validation, with CheckFinalTx() being what you’d expect people to normally be using to answer the question “Is the transaction (effectively) final right now?” (possibly IsFinalTx() might even get renamed to OldIsFinalTx() to further hammer home that point)

Now, at some point in the implementation of that BIP I wouldn’t be surprised if it makes sense to make it possible to provide a “chain tip” parameter to CheckFinalTx() to tell it what chain you’re checking with respect too, and thus what’s the median time, but for now the consensus-critical stuff can stay the way it is. Importantly, by changing everything else we get it all onto a new function with guaranteed semantics of “just figure out if this transaction is final according to whatever rules exist” - exactly what you’d expect from a Check*() function! Equally, when that chainTip parameter does get added, we’ll be sure to catch everything that needs to be updated in one fell swoop. (if it’s not done as a default parameter)

Anyway, smaller total diffs aren’t necessarily always a good thing. In this case, by changing the function name we help clue people in to think “Why did IsFinalTx() change names?” “What’s different about the design now?”, hopefully setting the stage to discover things like a future median time rule change.

jtimon commented at 3:10 pm on May 28, 2015: contributor

“just figure out if this transaction is final according to whatever rules exist” - exactly what you’d expect from a Check*() function!

I would also expect that function to be stateless!

Anyway, smaller total diffs aren’t necessarily always a good thing. In this case, by changing the function name we help clue people in to think “Why did IsFinalTx() change names?” “What’s different about the design now?”, hopefully setting the stage to discover things like a future median time rule change.

Then why keep the name IsFinalTx at all when the function is changing from global-state-dependent to stateless? What about using CheckFinalTx for the stateless one and finding anther name for the convenience new one? Like, I don’t know, TestFinalTx or something. That alone would make me happy.

Something you’re probably not aware of re: CheckFinalTx() is that I’ll be submitting another pull-req once this is merged to change CheckFinalTx() to use GetMedianTime() rather than GetAdjustedTime(). > This is important for a number of security related reasons. (most discussed before on #bitcoin-wizards) After that I’ll be proposing a soft-fork enforcing this within blocks themselves.

Ok, I didn’t know this. I have to say that passing a CBlockIndex* from the beginning instead of using the global tip would make the function more stateless and my concerns with it being named CheckFinalTx would be lower, specially if it’s going to be fully statelss later by not calling GetAdjustedTime(). What about passing a CBlockIndex from the beginning? This will not increase the total diff either. Maybe also moving to from GetAdjustedTime to GetMedianTime in this PR. Tat would make CheckFinalTx stateless from the beginning: that would make me happy as well.

So, to reiterate and in summary: a function named CheckFinalTx should be as stateless as possible from the beginning: using the global activeTip inside it, even temporarily, seems horrendous to me. I now realize that there’s more options than I was considering to make that happen.

jtimon commented at 3:18 pm on May 28, 2015: contributor

Note that for the last option, CheckFinalTx could optionally take the time (which callers that use it would set as GetAdjustedTime) which is set to pindexTip->GetMedianTime() inside if it’s not provided (or 0 is passed). Then your softfork would merely consist on removing the optional parameter to always use pindexTip->GetMedianTime. I believe that would be the clearest history possible for those plans. And I could also cleanly move both functions to consensus (do we still need 2 functions at this point?) in #6051 .

petertodd commented at 4:14 pm on May 28, 2015: contributor

Then why keep the name IsFinalTx at all when the function is changing from global-state-dependent to stateless?

Again, from the point of view of the code that mattered, IsFinalTx() was a stateless function… That it potentially wasn’t was an unfortunate bug, not a feature.

Equally, we can’t get rid of it after a soft-fork and replace it with some CheckFinalTx() or whatnot that takes block indexes, as the old behavior is still needed to verify old blocks.

Re: adding CBlockIndex’s and the like right now, that needs some discussion and thought, so it’s not going to happen in this pull-req.

jtimon commented at 4:35 pm on May 28, 2015: contributor

Re: adding CBlockIndex’s and the like right now, that needs some discussion and thought, so it’s not going to happen in this pull-req.

At least moving up the use of the global chainActive shouldn’t generate much discussion. Forcing the callers to call GetAdjustedTime themselves shouldn’t be controversial either, specially since you’re already touching those lines. At that point, I would be happy with the new one being named CheckFinalTx or (even Consensus::CheckFinalTx) because it would already be stateless (even without using GetMedianTime). Even not forcing them to call GetAdjustedTime would be acceptable, but there’s really no good reason to use chainActive inside a new function no matter how it is named.

Equally, we can’t get rid of it after a soft-fork and replace it with some CheckFinalTx() or whatnot that takes block indexes, as the old behavior is still needed to verify old blocks.

As said I would be happy with both:

Maintaining it as CheckFinalTx (new name because nw it’s stateless) and maybe unify it with the new TestFinalTx in the future.
If the new function is going to be stateless and named CheckFinalTx, we can maintain it as IsFinalTx or IsFinalTxOld or something.

So even assuming we want to have 2 separate functions for now, there’s at least 2 options for getting a stateless (consensus-friendly) one named CheckFinalTx in this PR (so I would be happy to close #6063 and rebase #6051 on top of this).

petertodd commented at 7:12 pm on May 28, 2015: contributor

Forcing the callers to call GetAdjustedTime themselves shouldn’t be controversial either, specially since you’re already touching those lines.

If I did that, we’d have to re-touch a whole bunch of code all over again later… Notably, that’s the kind of design mistake that lead to the bug this pull-req is fixing (re)appearing in the first place.

Anyway, continuing this discussion is a waste of time. @laanwj do whatever you want, just make sure this bug actually gets fixed.

jtimon commented at 9:41 pm on May 28, 2015: contributor

If I did that, we’d have to re-touch a whole bunch of code all over again later… Notably, that’s the kind of design mistake that lead to the bug this pull-req is fixing (re)appearing in the first place.

Fair enough, don’t do that then. Can we at least avoid the gratuitous use of the global chainActive in the new function?

laanwj merged this on Jun 1, 2015

laanwj closed this on Jun 1, 2015

laanwj referenced this in commit 87550eefc1 on Jun 1, 2015

laanwj commented at 9:36 am on June 1, 2015: member

ACK. Merging this as-is. Can discuss naming later but I think the naming is fine. (I was about to suggest that the new CheckFinalTx can be moved to the wallet, but unfortunately there’s one use left in main.cpp itself in AcceptToMemoryPool)

laanwj referenced this in commit 75a4d512cf on Jun 1, 2015

MarcoFalke locked this on Sep 8, 2021

Fix off-by-one error w/ nLockTime in the wallet #6183