policy: Remove unused locktime flags #24080

maflcko · 2022-01-16T10:24:46Z

The locktime flags have many issues:

They are passed in by a default argument, which is fragile. It has already lead to bugs like the one fixed in commit e30b6ea.
They are negative (signed), which doesn't make sense for flags (unsigned in general). According to the review comments when the code was added: "The max on the flags is a fairly weird operation." (BIP-113: Mempool-only median time-past as endpoint for lock-time calculations #6566 (comment))
No call site relies on the default argument and they all pass in a single compile-time constant, rendering most of the code dead and untested.
The dead code calls GetAdjustedTime (network adjusted time), which has its own issues. See AddTimeData will never update nTimeOffset past 199 samples #4521

Fix all issues by removing them

maflcko · 2022-01-16T10:26:09Z

Please don't review yet. This happens to compile, but depends on #24067 to be safe.

DrahtBot · 2022-01-16T14:49:26Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24538 (miner: bug fix? update for ancestor inclusion using modified fees, not base by glozow)
#23897 (refactor: Separate lockpoint calculate logic from CheckSequenceLocks function by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This checks finality at the current Tip, so clarify this in its name. -BEGIN VERIFY SCRIPT- ren() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/ ) ; } ren CheckSequenceLocks CheckSequenceLocksAtTip ren CheckFinalTx CheckFinalTxAtTip -END VERIFY SCRIPT-

theStack · 2022-02-25T17:36:26Z

Concept ACK

theStack

Code-review ACK fa8d4d9

Verified that the issues stated in the PR description are fixed and that the commits don't change any behaviour.

ajtowns · 2022-03-11T14:12:03Z

ACK fa8d4d9

I split up the first commit quite a bit to see what was going on; branch is here in case that makes anyone else's review easier. I didn't notice all the CTransaction{tx} changes without it...

Bit surprised at calling them "policy" functions -- finality is a consensus question; but this does impact the mempool, and when we generate blocks from the mempool we check finality again there too.

maflcko · 2022-03-11T15:06:39Z

Bit surprised at calling them "policy" functions -- finality is a consensus question; but this does impact the mempool, and when we generate blocks from the mempool we check finality again there too.

I call them policy, because they don't affect the validity of blocks. They can at most affect validitity of txs added to the mempool.

ajtowns · 2022-03-11T17:40:07Z

I call them policy, because they don't affect the validity of blocks. They can at most affect validitity of txs added to the mempool.

Sorry, I didn't word that very well. Let me start again from scratch, since I want to add something anyway.

My concern was that these functions might be consensus critical, in that if you don't check that they're correct when you put transactions into the mempool, you might then see them in the mempool and populate a block template with them, and after mining the block discover that the block is consensus invalid. (Hopefully that logic is obvious, as far as it goes!)

However, I was thinking that that turns out to be okay because node/miner.cpp has BlockAssembler::TestPackageTransactions double checks IsFinalTx before allowing txs to be added to a block (template). Which would agree with what you say above: it doesn't affect the validity of blocks.

But, looking again, as far as I can see, node/miner.cpp is only checking IsFinalTx; it's not also checking nSequence or calling CalculateSequenceLocks? If that's the case, then if a tx that doesn't pass CheckSequenceLocks makes it into the mempool, I think we would generate invalid blocks as a result.

So I think that means CalculateSequenceLocks is effectively consensus critical, at least for nodes trying to generate blocks for mining.

(EDIT: this isn't invalidating my ACK, just critiquing the comment in the header, and checking my understanding of what's going on)

maflcko · 2022-03-11T17:52:32Z

I think this is true for any mempool logic. If priority had a bug to allow txs with negative fee into the mempool, it would also kill the miner.

Not sure what to do now. I can replace "policy" with "mempool" in the pull request subject line?

ajtowns · 2022-03-11T18:19:30Z

I think this is true for any mempool logic. If priority had a bug to allow txs with negative fee into the mempool, it would also kill the miner.

I'd call it a consensus bug if we accepted a tx with negative fee into the mempool (and certainly a consensus bug if that tx then made it into a block template).

Some parts of mempool acceptance are consensus critical, because we don't re-check them when accepting a block; some parts are consensus critical because we don't re-check them when generating a block; in my opinion, "policy" is any remaining things that wouldn't result in an invalid block being generated/accepted. At the moment, as I understand it, CheckFinalTx is just policy, but CheckSequenceLocks is consensus critical at least when generating blocks?

Not sure what to do now. I can replace "policy" with "mempool" in the pull request subject line?

I don't think anything needs to be done right now. Could consider adding a SequenceLocks() check to node/miner.cpp later?

glozow · 2022-03-14T14:48:17Z

I don't think anything needs to be done right now. Could consider adding a SequenceLocks() check to node/miner.cpp later?

Is it insufficient to be using TestBlockValidity() for block templates created in the miner?

bitcoin/src/node/miner.cpp

Line 170 in 25d045a

    
           if (!TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev, false, false)) {

SequenceLocks() is called for every transaction in TestBlockValidity() through ConnectBlock():

bitcoin/src/validation.cpp

Line 2167 in 25d045a

if (!SequenceLocks(tx, nLockTimeFlags, prevheights, *pindex)) {

ajtowns · 2022-03-14T14:54:54Z

Is it insufficient to be using TestBlockValidity() for block templates created in the miner?

In so far as you want getblocktemplate to give you something you can mine, rather than an error, yeah? (Better than giving an invalid template and not discovering until after you've mined an otherwise valid block though)

glozow

ACK fa8d4d9, agree the default arg flags is a massive footgun and just setting max flags is weird. Adding AtTip to the names makes sense to me, since they're both testing for next block and only ever used for {,re}addition to mempool.

glozow · 2022-03-14T15:40:43Z

In so far as you want getblocktemplate to give you something you can mine, rather than an error, yeah? (Better than giving an invalid template and not discovering until after you've mined an otherwise valid block though)

Oh okay I see what you're saying, yes, it will catch an invalid template but won't give you a valid one. I actually don't know what the next step would be if you're a miner and have generated an invalid template because there's an invalid transaction in the mempool. I guess you need to delete mempool.dat and restart (and hopefully file a bug report). Or you could prioritisetransaction(-MAXMONEY) the transaction that failed and generate another template. But not recovering easily actually seems appropriate to me, since this would be a pretty bad bug?

maflcko · 2022-03-14T15:43:33Z

Changed back to "validation" from "policy" in follow-up #24564

ajtowns · 2022-03-14T15:46:21Z

Yeah, it's a pretty bad bug if getblocktemplate can't give you a template to mine on; it's just a worse bug if it silently gives you an invalid one. So the solution is either for the mempool to guarantee there are no not-yet-final Sequence Lock transactions in it (in which case that's not just a "policy" matter), or for getblocktemplate to do the same thing for sequence lock transactions it does for !IsFinalTx ones, and skip them (and any cpfp ancestors) if they end up in the mempool somehow.

fa8d4d9 scripted-diff: Clarify CheckFinalTxAtTip name (MarcoFalke) fa4e30b policy: Remove unused locktime flags (MarcoFalke) Pull request description: The locktime flags have many issues: * They are passed in by a default argument, which is fragile. It has already lead to bugs like the one fixed in commit e30b6ea. * They are negative (signed), which doesn't make sense for flags (unsigned in general). According to the review comments when the code was added: "The max on the flags is a fairly weird operation." (bitcoin#6566 (comment)) * No call site relies on the default argument and they all pass in a single compile-time constant, rendering most of the code dead and untested. * The dead code calls `GetAdjustedTime` (network adjusted time), which has its own issues. See bitcoin#4521 Fix all issues by removing them ACKs for top commit: ajtowns: ACK fa8d4d9 theStack: Code-review ACK fa8d4d9 glozow: ACK fa8d4d9, agree the default arg `flags` is a massive footgun and just setting max flags is weird. Adding `AtTip` to the names makes sense to me, since they're both testing for *next* block and only ever used for {,re}addition to mempool. Tree-SHA512: 79f4a52f34909eb598d88bbae7afe8abe5f85f45c128483d16aa83dacd0e5579e561b725d01b1e9a931d1821012a51ad2bc6fb2867f8d09ee541f9d234d696f8

fa1fe2e Remove LOCKTIME_MEDIAN_TIME_PAST constant (MarcoFalke) Pull request description: The constant is exposed in policy code, which doesn't make sense: * Wallet and mempool need to assume the flag to be always active to function properly. * Setting (or unsetting) the flag has no effect on policy code. The constant is only used in `ContextualCheckBlock` (consensus code) to set a flag and then read the flag again. I think this can be better achieved by using a `bool`. If there is a need to use a flag in the future, it will be trivial to do so then. (The previous use for the constant was removed in df562d6) ACKs for top commit: Sjors: utACK fa1fe2e glozow: code review ACK fa1fe2e, AFAICT this is safe and makes sense as `SequenceLocks` doesn't use it, wallet/ATMP no longer need it since #24080, and `ContextualCheckBlock` effectively uses it as a roundabout boolean. instagibbs: utACK fa1fe2e Tree-SHA512: de1972498c545d608a09630d77d8c7e38ed50a6ec40d6c0d720310a1647ed5b48b4ace0078c80db10e7f97aacc552fffae251fe3256e9a19a908b933ba2dc552

fa1fe2e Remove LOCKTIME_MEDIAN_TIME_PAST constant (MarcoFalke) Pull request description: The constant is exposed in policy code, which doesn't make sense: * Wallet and mempool need to assume the flag to be always active to function properly. * Setting (or unsetting) the flag has no effect on policy code. The constant is only used in `ContextualCheckBlock` (consensus code) to set a flag and then read the flag again. I think this can be better achieved by using a `bool`. If there is a need to use a flag in the future, it will be trivial to do so then. (The previous use for the constant was removed in df562d6) ACKs for top commit: Sjors: utACK fa1fe2e glozow: code review ACK fa1fe2e, AFAICT this is safe and makes sense as `SequenceLocks` doesn't use it, wallet/ATMP no longer need it since bitcoin#24080, and `ContextualCheckBlock` effectively uses it as a roundabout boolean. instagibbs: utACK fa1fe2e Tree-SHA512: de1972498c545d608a09630d77d8c7e38ed50a6ec40d6c0d720310a1647ed5b48b4ace0078c80db10e7f97aacc552fffae251fe3256e9a19a908b933ba2dc552

…on function fa86710 Clarify that CheckSequenceLocksAtTip is a validation function (MarcoFalke) Pull request description: It has been pointed out that a bug in this function can prevent block template creation. ( #24080 (comment) ) So it seems that the scope of this function is more than "policy". Rename it back to "validation", to partially revert commit fa4e30b. ACKs for top commit: ajtowns: ACK fa86710 - looks fine to me glozow: ACK fa86710 Tree-SHA512: 2e0df8c70df4cbea857977f140a8616cfa7505e74df66c9c9fbcf184670ce3ce7567183c3f76e6f3fe8ca6de0e065b9babde6352d6cb495e71ea077ddedbc3f4

…alidation function fa86710 Clarify that CheckSequenceLocksAtTip is a validation function (MarcoFalke) Pull request description: It has been pointed out that a bug in this function can prevent block template creation. ( bitcoin#24080 (comment) ) So it seems that the scope of this function is more than "policy". Rename it back to "validation", to partially revert commit fa4e30b. ACKs for top commit: ajtowns: ACK fa86710 - looks fine to me glozow: ACK fa86710 Tree-SHA512: 2e0df8c70df4cbea857977f140a8616cfa7505e74df66c9c9fbcf184670ce3ce7567183c3f76e6f3fe8ca6de0e065b9babde6352d6cb495e71ea077ddedbc3f4

Summary: ``` The locktime flags have many issues: They are passed in by a default argument, which is fragile. It has already lead to bugs like the one fixed in commit e30b6ea. They are negative (signed), which doesn't make sense for flags (unsigned in general). According to the review comments when the code was added: "The max on the flags is a fairly weird operation." ( BIP-113: Mempool-only median time-past as endpoint for lock-time calculations #6566 (comment)) No call site relies on the default argument and they all pass in a single compile-time constant, rendering most of the code dead and untested. The dead code calls GetAdjustedTime (network adjusted time), which has its own issues. See AddTimeData will never update nTimeOffset past 199 samples #4521 Fix all issues by removing them ``` Backport of [[bitcoin/bitcoin#24080 | core#24080]]. Depends on D12949 and D12950. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Subscribers: sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12951

maflcko changed the title ~~refactor: Remove unused locktime flags~~ policy: Remove unused locktime flags Jan 16, 2022

maflcko added Refactoring TX fees and policy labels Jan 16, 2022

maflcko force-pushed the 2201-lockstuff branch from 902e4aa to f5fa07c Compare January 19, 2022 14:38

DrahtBot added the Needs rebase label Jan 25, 2022

maflcko force-pushed the 2201-lockstuff branch from f5fa07c to fa17b37 Compare January 26, 2022 06:48

maflcko marked this pull request as ready for review January 26, 2022 06:48

DrahtBot removed the Needs rebase label Jan 26, 2022

MarcoFalke added 2 commits January 27, 2022 08:46

policy: Remove unused locktime flags

fa4e30b

maflcko force-pushed the 2201-lockstuff branch from fa17b37 to fa8d4d9 Compare January 27, 2022 07:51

theStack approved these changes Feb 27, 2022

View reviewed changes

DrahtBot mentioned this pull request Mar 12, 2022

miner: bug fix? update for ancestor inclusion using modified fees, not base #24538

Merged

glozow reviewed Mar 14, 2022

View reviewed changes

maflcko merged commit 28bdaa3 into bitcoin:master Mar 14, 2022

maflcko mentioned this pull request Mar 14, 2022

doc: Clarify that CheckSequenceLocksAtTip is a validation function #24564

Merged

maflcko deleted the 2201-lockstuff branch March 14, 2022 15:48

maflcko mentioned this pull request Mar 19, 2022

Remove LOCKTIME_MEDIAN_TIME_PAST constant #24565

Merged

bitcoin locked and limited conversation to collaborators Jun 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

policy: Remove unused locktime flags #24080

policy: Remove unused locktime flags #24080

Uh oh!

maflcko commented Jan 16, 2022 •

edited

Loading

Uh oh!

maflcko commented Jan 16, 2022

Uh oh!

DrahtBot commented Jan 16, 2022 •

edited

Loading

Uh oh!

theStack commented Feb 25, 2022

Uh oh!

theStack left a comment

Uh oh!

ajtowns commented Mar 11, 2022

Uh oh!

maflcko commented Mar 11, 2022

Uh oh!

ajtowns commented Mar 11, 2022 •

edited

Loading

Uh oh!

maflcko commented Mar 11, 2022

Uh oh!

ajtowns commented Mar 11, 2022

Uh oh!

glozow commented Mar 14, 2022

Uh oh!

ajtowns commented Mar 14, 2022

Uh oh!

glozow left a comment

Uh oh!

glozow commented Mar 14, 2022

Uh oh!

maflcko commented Mar 14, 2022

Uh oh!

ajtowns commented Mar 14, 2022

Uh oh!

Uh oh!

policy: Remove unused locktime flags #24080

policy: Remove unused locktime flags #24080

Uh oh!

Conversation

maflcko commented Jan 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jan 16, 2022

Uh oh!

DrahtBot commented Jan 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

theStack commented Feb 25, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Mar 11, 2022

Uh oh!

maflcko commented Mar 11, 2022

Uh oh!

ajtowns commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Mar 11, 2022

Uh oh!

ajtowns commented Mar 11, 2022

Uh oh!

glozow commented Mar 14, 2022

Uh oh!

ajtowns commented Mar 14, 2022

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

glozow commented Mar 14, 2022

Uh oh!

maflcko commented Mar 14, 2022

Uh oh!

ajtowns commented Mar 14, 2022

Uh oh!

Uh oh!

maflcko commented Jan 16, 2022 •

edited

Loading

DrahtBot commented Jan 16, 2022 •

edited

Loading

ajtowns commented Mar 11, 2022 •

edited

Loading