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

Merged
merged 1 commit into from Jun 1, 2015

Conversation

Projects
None yet
4 participants
@petertodd
Contributor

petertodd commented May 25, 2015

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 665bdd3, 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.

@laanwj laanwj added the Bug label May 26, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj 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.
Member

laanwj commented May 26, 2015

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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr 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.

Member

luke-jr commented May 26, 2015

@laanwj +1

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 27, 2015

Member

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

Member

laanwj commented May 27, 2015

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 665bdd3,
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.
@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd 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

// FIXME: we should *actually* create a new block so the following test
?

Contributor

petertodd commented May 27, 2015

@laanwj Split into two functions.

In doing so I also had to add part #6177

Any thoughts on what should happen at

// FIXME: we should *actually* create a new block so the following test
?

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon May 27, 2015

Member

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

// FIXME: we should *actually* create a new block so the following test
?

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

Member

jtimon commented May 27, 2015

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

// FIXME: we should *actually* create a new block so the following test
?

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

- BOOST_CHECK(IsFinalTx(tx2));
+ // FIXME: we should *actually* create a new block so the following test
+ // works; CheckFinalTx() isn't fooled by monkey-patching nHeight.
+ //BOOST_CHECK(CheckFinalTx(tx));

This comment has been minimized.

@laanwj

laanwj May 27, 2015

Member

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

@laanwj

laanwj May 27, 2015

Member

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

This comment has been minimized.

@petertodd

petertodd May 27, 2015

Contributor

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.

@petertodd

petertodd May 27, 2015

Contributor

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.

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

This comment has been minimized.

@laanwj

laanwj May 27, 2015

Member

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

@laanwj

laanwj May 27, 2015

Member

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj 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.

Member

laanwj commented May 27, 2015

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.

if (tx.nLockTime == 0)
return true;
- if (nBlockHeight == 0)

This comment has been minimized.

@jtimon

jtimon May 27, 2015

Member

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.

@jtimon

jtimon May 27, 2015

Member

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.

This comment has been minimized.

@petertodd

petertodd May 27, 2015

Contributor

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.

@petertodd

petertodd May 27, 2015

Contributor

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.

This comment has been minimized.

@jtimon

jtimon May 28, 2015

Member

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).

@jtimon

jtimon May 28, 2015

Member

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).

This comment has been minimized.

@petertodd

petertodd May 28, 2015

Contributor

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.

@petertodd

petertodd May 28, 2015

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon May 27, 2015

Member

@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.

Member

jtimon commented May 27, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd 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.

Contributor

petertodd commented May 27, 2015

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

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon May 28, 2015

Member

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.

  1. 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).

  2. Separate a stateless (consensus-friendly) version of the function, like in #6063 .

  3. 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).

  4. 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).

  5. 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.

  6. 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).

Member

jtimon commented May 28, 2015

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.

  1. 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).

  2. Separate a stateless (consensus-friendly) version of the function, like in #6063 .

  3. 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).

  4. 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).

  5. 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.

  6. 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

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd 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.

Contributor

petertodd commented May 28, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon May 28, 2015

Member

"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.

Member

jtimon commented May 28, 2015

"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

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon May 28, 2015

Member

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 .

Member

jtimon commented May 28, 2015

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

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd 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.

Contributor

petertodd commented May 28, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon May 28, 2015

Member

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:

  1. Maintaining it as CheckFinalTx (new name because nw it's stateless) and maybe unify it with the new TestFinalTx in the future.

  2. 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).

Member

jtimon commented May 28, 2015

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:

  1. Maintaining it as CheckFinalTx (new name because nw it's stateless) and maybe unify it with the new TestFinalTx in the future.

  2. 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

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd 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.

Contributor

petertodd commented May 28, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon May 28, 2015

Member

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?

Member

jtimon commented May 28, 2015

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 laanwj merged commit 28bf062 into bitcoin:master Jun 1, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 1, 2015

Merge pull request #6183
28bf062 Fix off-by-one error w/ nLockTime in the wallet (Peter Todd)
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 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)

Member

laanwj commented Jun 1, 2015

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 added a commit that referenced this pull request Jun 1, 2015

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 665bdd3,
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.

Github-Pull: #6183
Rebased-From: 28bf062
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment