policy: Remove unused locktime flags

MarcoFalke commented at 10:24 am on January 16, 2022: member

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 e30b6ea194fee3bb95a45e7b732a99566b88f1f5.
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.” (https://github.com/bitcoin/bitcoin/pull/6566#issuecomment-150310861)
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 #4521

Fix all issues by removing them

MarcoFalke commented at 10:26 am on January 16, 2022: member

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

MarcoFalke renamed this:
~~refactor: Remove unused locktime flags~~
policy: Remove unused locktime flags
on Jan 16, 2022

MarcoFalke added the label Refactoring on Jan 16, 2022

MarcoFalke added the label TX fees and policy on Jan 16, 2022

DrahtBot commented at 2:49 pm on January 16, 2022: member

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.

MarcoFalke force-pushed on Jan 19, 2022

DrahtBot added the label Needs rebase on Jan 25, 2022

MarcoFalke force-pushed on Jan 26, 2022

MarcoFalke marked this as ready for review on Jan 26, 2022

DrahtBot removed the label Needs rebase on Jan 26, 2022

policy: Remove unused locktime flags fa4e30b0f3

scripted-diff: Clarify CheckFinalTxAtTip name

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-

fa8d4d9128

MarcoFalke force-pushed on Jan 27, 2022

theStack commented at 5:36 pm on February 25, 2022: member

Concept ACK

theStack approved

theStack commented at 3:58 pm on February 27, 2022: member

Code-review ACK fa8d4d9128c35de0fe715f2e2b99269d23c09cc1

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

laanwj added this to the "Blockers" column in a project

ajtowns commented at 2:12 pm on March 11, 2022: member

ACK fa8d4d9128c35de0fe715f2e2b99269d23c09cc1

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.

MarcoFalke commented at 3:06 pm on March 11, 2022: member

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 commented at 5:40 pm on March 11, 2022: member

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)

MarcoFalke commented at 5:52 pm on March 11, 2022: member

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 commented at 6:19 pm on March 11, 2022: member

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 commented at 2:48 pm on March 14, 2022: member

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? https://github.com/bitcoin/bitcoin/blob/25d045a9ecc24a2752685d176c28f8f75bb100f6/src/node/miner.cpp#L170

SequenceLocks() is called for every transaction in TestBlockValidity() through ConnectBlock(): https://github.com/bitcoin/bitcoin/blob/25d045a9ecc24a2752685d176c28f8f75bb100f6/src/validation.cpp#L2167

ajtowns commented at 2:54 pm on March 14, 2022: member

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 commented at 3:13 pm on March 14, 2022: member

ACK fa8d4d9128c35de0fe715f2e2b99269d23c09cc1, 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.

MarcoFalke merged this on Mar 14, 2022

MarcoFalke closed this on Mar 14, 2022

glozow commented at 3:40 pm on March 14, 2022: member

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?

MarcoFalke commented at 3:43 pm on March 14, 2022: member

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

ajtowns commented at 3:46 pm on March 14, 2022: member

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.

MarcoFalke deleted the branch on Mar 14, 2022

sidhujag referenced this in commit 0daf624c9e on Mar 14, 2022

laanwj removed this from the "Blockers" column in a project

fanquake referenced this in commit bace615ba3 on Jun 28, 2022

sidhujag referenced this in commit 57a86429a3 on Jun 28, 2022

glozow referenced this in commit c012875b9d on Aug 9, 2022

sidhujag referenced this in commit 89c82d26b8 on Aug 9, 2022

Fabcien referenced this in commit 1c4d06f75d on Jan 5, 2023

DrahtBot locked this on Jun 11, 2023

policy: Remove unused locktime flags #24080

Conflicts