[RFC] enable `clang-tidy` `optin.performance.Padding` #36819

fanquake commented at 5:49 PM on November 9, 2026: member

Enable optin.performance.Padding with the default of 24 bytes (made explicit in the clang-tidy config), after fixing warnings for more padding than that. Enabling it at this point would be to catch regressions above this amount.

Based on #36501, which reorders CBlockIndex to save 8 bytes (clang-tidy also warns about this). Wondering if contributors/reviewers think this is worthwhile given we may make that refactor? Also not sure if it's possible for us to enable this generally, as padding will differ depending on various factors.

Reorder CBlockIndex members to save 8 bytes

On a windows x64 build, CBlockIndex size is 160 bytes. Moving one field
somewhere else makes the class lose less bytes due to padding to respect
the 8-byte alignment

For a ~1M block height, this saves about 8M of Ram without doing
anything else.

c2dedba48f

Reorder CNode and CConnman members to reduce padding e90a267db7

Reorder CTxMemPool members to reduce padding db7b3535c5

wallet: reorder CWalletTx members to reduce padding f6879ec26a

net_processing: reorder Peer members to reduce padding 5d72e30b3a

rpc: reorder CRPCConvertParam members to reduce padding 0c4d15f589

clang-tidy: add clang-analyzer-optin.performance.Padding

In #36501 we reordered fields in `CBlock` to save 8 bytes.
Add the clang-tidy padding check, leave the warning threshold
(explicitly) at the default of 24 bytes.

https://clang.llvm.org/docs/analyzer/checkers.html#optin-performance-padding

c2f15bb647

DrahtBot commented at 5:49 PM on November 9, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36819
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36819.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36819#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	sedited, jonatack, hodlinator, bartoli

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36736 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36736.svg"> (net: unify send pause state by l0rinc)
#35940 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35940.svg"> (net: allow selecting BIP152 high-bandwidth peers with -addnode by w0xlt)
#35027 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35027.svg"> (net: add -outboundbind option for outgoing source address by 8144225309)
#27865 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/27865.svg"> (wallet: Track no-longer-spendable TXOs separately by achow101)

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.

sedited commented at 6:16 PM on November 9, 2026: contributor

Concept ACK, seems worthwhile to me.

jonatack commented at 6:30 PM on November 9, 2026: member

Concept ACK, modulo consider keeping guarded members near their mutex and avoiding churn or grouping members less logically where it provides negligible benefit (arguably the changes in rpc/client.cpp and txmempool.h).

hodlinator commented at 6:40 PM on November 9, 2026: contributor

Concept ACK, probably good to turn this on by default with a reasonable threshold.

For singleton types (*Man etc), in cases where it really leads to less logical groupings of fields, one could opt out, like we do for recursion warnings today.

DrahtBot added the label CI failed on Nov 9, 2026

DrahtBot commented at 6:51 PM on November 9, 2026: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/42668387091/job/129338067669 LLM reason (✨ experimental): CI failed because clang-tidy flagged excessive struct padding in ValidVector in test/bip352_tests.cpp; warnings are treated as errors.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

bartoli commented at 9:00 PM on November 9, 2026: none

Concept ACK for me too. I had not included other classes in the original PR because they are probably instantiated far less than 1 million times at once, so the changes seemed not worth it to me (sometimes members are ordered by fonctionality, code might be more understandable that way). But if there is now a CI check to avoid potentially losing more memory in the future, I'm all for it.

It seems the limit of 23 is not properly used though, since the CI output warns about a loss of 11 padding bytes?

in src/rpc/client.cpp:29 in c2f15bb647

  21 | @@ -22,10 +22,13 @@ enum ParamFormat { JSON, STRING, JSON_OR_STRING };
  22 |  class CRPCConvertParam
  23 |  {
  24 |  public:
  25 | +    CRPCConvertParam(std::string method_name, int param_idx, std::string param_name, ParamFormat param_format = ParamFormat::JSON)
  26 | +        : methodName{std::move(method_name)}, paramName{std::move(param_name)}, paramIdx{param_idx}, format{param_format} {}
  27 | +
  28 |      std::string methodName; //!< method whose params want conversion
  29 | -    int paramIdx;           //!< 0-based idx of param to convert
  30 |      std::string paramName;  //!< parameter name

maflcko commented at 7:08 AM on November 10, 2026:

if memory usage was a concern here, those should be string_view

maflcko commented at 7:16 AM on November 10, 2026: member

Is this really worth it? This could make the code harder to read by having fields that belong together split further apart. See e.g. the mempool "rolling" section.

I'd say this should be an opt-in list for structs where we know they exist 1000s of times and that would benefit low-resource systems. E.g. saving 24kB on systems that have set 2000 connections (https://github.com/bitcoin/bitcoin/pull/35919), so likely they are not low-memory systems, seems a rounding error.

So my preference would be to have this opt-in per-struct and accommodate each change with a measurement, or at least a guess of how much it would save.

in src/txmempool.h:200 in c2f15bb647

 196 |      uint64_t cachedInnerUsage GUARDED_BY(cs){0}; //!< sum of dynamic memory usage of all the map elements (NOT the maps themselves)
 197 |  
 198 |      mutable int64_t lastRollingFeeUpdate GUARDED_BY(cs){GetTime()};
 199 | -    mutable bool blockSinceLastRollingFeeBump GUARDED_BY(cs){false};
 200 |      mutable double rollingMinimumFeeRate GUARDED_BY(cs){0}; //!< minimum fee to get into the pool, decreases exponentially
 201 |

maflcko commented at 7:17 AM on November 10, 2026:

splitting this up makes the code harder to read for no benefit?

maflcko commented at 7:17 AM on November 10, 2026: member

.

l0rinc commented at 1:20 PM on November 12, 2026: contributor

Not sure the tidy check makes sense (as @maflcko also mentioned, for classes with only a few instances in total, readability can matter more), but we could keep this open as a guide for similar changes, so we can consider them on a case-by-case basis.