policy: Remove unused locktime flags #24080

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

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

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

Concept ACK

Code-review ACK fa8d4d9128c35de0fe715f2e2b99269d23c09cc1

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

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.

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.

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)

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?

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?

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

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)

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.

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?

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

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.