refactor: consensus/tx_verify.{h,cpp} tidy-ups #24833

1. Fix lockpair in-param passing in EvaluateSequenceLocks() and other tidy-ups:

   • pass lockPair by reference to const instead of by value, per https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-in

   • make nBlockTime const

   • rename lockPair to lock_pair

   • rename nBlockTime to block_time

   • add brackets to conditionals

2. add consensus/tx_verify.{h,cpp} to the iwyu (include what you use) CI checks, along with missing include headers to satisfy the checks

3. apply clang-format to consensus/tx_verify.{h,cpp}

ACK https://github.com/bitcoin/bitcoin/pull/24833/commits/e6dde9a3e7aeb2eeb00950dd3a7231ae94d33017

But why is the first commit (https://github.com/bitcoin/bitcoin/pull/24833/commits/0b095dd52e846ae7c228bf52144d3470c34b00da) necessary ?

Thanks @w0xlt! If your question is why are they considered to be missing, see doc/developer-notes.md:

- Every `.cpp` and `.h` file should `#include` every header file it directly uses classes, functions or other
  definitions from, even if those headers are already included indirectly through other headers.

  - *Rationale*: Excluding headers because they are already indirectly included results in compilation
    failures when those indirect dependencies change. Furthermore, it obscures what the real code
    dependencies are.

@jonatack Thanks for the explanation.

<!--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:

• #24567 (Enforce BIP 68 from genesis by MarcoFalke)

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 is basically just formatting changes? Confused by calling it "fixups" - does it fix anything?

It seemed like a good description for missing headers and how lockPair is passed; the last one is formatting.

I find the include ones hard to review and there doesn't seem to be any benefit absent a report that compilation fails. If this is something we want to do, it might be better done via #24831. Otherwise we'll have daily pulls to "fix" the includes, remove random ones and add random ones without affecting the resulting binary and only causing merge conflicts.

This has enough review for merge? See #24740 for a similar recent example.

(#24831 is a POC/draft; if it were to be merged at some point, we would still need to review the changes and this reduces the burden as that has already been done here.)

(If you disagree, please LMK how best to proceed most productively.)

and this reduces the burden as that has already been done here.

This would still need to be touched again, as the includes fixups are not comprehensive. i.e: <cassert> and <cstdint> are missing, among others.

This has enough review for merge? See #24740 for a similar recent example.

#24740 actually fixed an incorrect comment, whereas this pull doesn't fix anything that is wrong?

I don't have a strong opinion, but I still think we should first try iwyu and defer to it instead of doing it with manual review work. In the meantime it might be best to not touch the include stuff unless there is a reproducible compile error.

Not sure what to do here. 0b095dd52e846ae7c228bf52144d3470c34b00da should probably either be updated to add all the missing includes, so we don't have to touch them again for IWYU, or be dropped. Besides that, as others have commented, it's not clear what is being fixed, given nothing seems to be broken, so maybe wordings can be improved.

Sorry for not updating sooner. Added a cassert include to the cpp file and renamed the pull and second commit from fixups to tidy-ups. Happy to drop the first commit if preferred.

Rebased.

Squashed the three commits down to one.

This now contains unrelated changes. Looks like from a rebase.

Thanks, which changes? I verified that a rebase to master would be clean but didn't rebase.

Ah, don't worry, looks like it's just a GitHub bug.

Cool. Happy to drop some of the diff if people prefer.

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.