💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314667768)
Done

💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314668014)
Hm, I am not sure on this one. It sounds like this is so unlikely that the overflow value would probably go without an implementation of how to use it until something that triggers this actually happens. E.g. even if something goes on that looks suspicious the next release may be a few months out so the attack could already be carried out. In that case users would need to resync again anyway if I’m not mistaken. So I would prefer to only add this if there is a clear path to implement the overflo
...

💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314668306)
Right, this code is "belts-and-suspenders" since we should never end up here since the base index should rewind us in this case before entering `CustomAppend`. So I will turn this into an error and drop the hashkey query below since we aren't recovering from that anyway.

This isn't something that is added here so it could be a separate PR but I will add it here since it simplifies the code a bit.

💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314668493)
> Adding {0} default initializers in the struct DBVal declaration seems sensible?

Done

💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314670363)
Ok, this basically means we are only turning the values into per-block that are most in danger of overflowing. I have done this and since we are not removing most of the members anymore, I squashed that commit which I hope should make review a bit easier now as well. At least overall the change is simpler with this, I think.

💬 fjahr commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3243423289)
> When running this index over signet, I end up with ~700 55kB ldb files, and a single 8.8MB ldb file. That seems like something is probably suboptimal?

This doesn’t seem to be caused by this change afaict, my old coinstatsindex on signet has the same file structure. I have tried to find the cause for this but so far I can’t really say for sure where the difference to the other indexes lies. There does not seem to be any difference in configuration and I looked into additional batching db wri
...

💬 ajtowns commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2314824654)
You only need 65 bits to handle a 1MB block full of 61B txs cycling 21M BTC, so could:
* add a single uint8_t that we'd use two bits from for each
* add two uint16_t's to count the number of times MAX_MONEY is exceeded
* use __int128 or arith_uint256 for those fields

Could also just cap the value:

```c++
static inline void cap_add(CAmount& v, CAmount x)
{
if (std::numeric_limits<CAmount>::max() - x < v) {
v = std::numeric_limits<CAmount>::max();
} else {

...

💬 frankomosh commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2314870867)
Had only kept it for readability, but yes we can definately do without it

💬 frankomosh commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2314871367)
yes. thanks

💬 l0rinc commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2314939752)
It looks like this branch isn't needed, in which case we can always just `try_emplace` (and add a dead-code erase for the tests), see [`ae76ec7` (#30673)](https://github.com/bitcoin/bitcoin/pull/30673/commits/ae76ec7bcff0a08a61f294882a71e46d177b009f#diff-f0ed73d62dae6ca28ebd3045e5fc0d5d02eaaacadb4c2a292985a3fbd7e1c77cR184)

I've pushed it to my own fork for now (https://github.com/l0rinc/bitcoin/pull/34), adding you and @andrewtoth as coauthors. Will publish once my benchmarks finish.

💬 maflcko commented on issue "InitError() doesn't always halt node startup when blockchain state exists":
(https://github.com/bitcoin/bitcoin/issues/33276#issuecomment-3243949312)
I can't reproduce this locally, but I guess you are referring to the `initload` thread doing its work? If yes, I presume the only fix would be to validate the args before starting the initload thread?

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2315050447)
No objection, but I am not sure we should diverge the subtree for a test-only patch. Also, the patch is pre-existing, so it seems a bit unrelated to this pull.

Ideally, Google would fix the UB in some way or another, and then we can pull whatever their fix was, but that shouldn't be a blocker here.

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2315050556)
fwd decls are slimmer than a full include, and often sufficient, for example when the header only uses the types as pointers or references. IWYU will suggest them sometimes, when possible.

💬 maflcko commented on issue "intermittent timeout in mptest unit test":
(https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3243976122)
At the same time, it seems to happen frequently in CI, so I prefer my initial suggestion to either rewrite the failing unit test or remove it temporarily.

💬 Sjors commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#issuecomment-3244007472)
> Now that the router has been fully reset without any lingering configs from prior major versions, PCP & NATPMP is actually working.

You would not be the first person running into router bugs while testing NAT punching functionality :-)

💬 Sjors commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315087718)
For a node that's been offline for a year or so and then caught up yesterday:

```
16G testnet3/chainstate/
195G testnet3/blocks/
210G total
```

💬 Sjors commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315089293)
Testnet4 node that's been online a couple of times, but not continuously:

```
938M testnet4/chainstate/
8,1G testnet4/blocks/
9,0G total
```

💬 l0rinc commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2315091137)
The main leveledb repo doesn't even compile for the past 8 months, not sure why we would wait for them. But as I mentioned, I don't mind doing it after the PR, just seems cleaner if we didn't need to change this section again.

💬 Sjors commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315092723)
And one that I spun up in June and his been online since:

```
915M testnet4/chainstate/
20G testnet4/blocks/
21G total
```

💬 l0rinc commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2315093684)
I know, that wasn't my question. But I'll figure it out next, feel free to resolve it.