💬 maflcko commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914788340)
> these values will be sent over IPC. At that point we have to fix their integer values. Might as well do it now?

Why would they need to be fixed? IPC is only an internal interface. It is currently experimental and in draft and there are no plans for the interface to support mismatching server/client binaries. I'd say the code should be left as-is until there are plans (with support) to even support that. Until then, changing the code for that reason seems like pointless churn to me.

💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914796771)
> no plans for the interface to support mismatching server/client binaries

That tends to happen without a plan, but indeed we can wait.

> part of the block index, or exposed via RPC

^ this still need to be clarified

👍 TheCharlatan approved a pull request: "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls"
(https://github.com/bitcoin/bitcoin/pull/31490#pullrequestreview-2549773423)
ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa

I don't see a change in the runtime of the benchmarks, but these are some good refactors that I have wanted for some time.

📝 maflcko opened a pull request: " lint: Call more checks from test_runner "
(https://github.com/bitcoin/bitcoin/pull/31653)
The lint `commit-script-check.sh` can not be called from the test_runner at all and must be called manually. Also, some checks require `COMMIT_RANGE` to be set.

Fix all issues by moving two lint checks into the test_runner. Also, the proper commit range is passed to the checks by the test_runner, so that the user no longer has to do it.

💬 marcofleon commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914880590)
I agree that if there were code to keep for now it would be this. I ultimately decided to remove it because the purpose it serves no longer seems to apply if we remove checkpoints. With the headers sync logic, iiuc, it would be fine for a node in IBD to respond to GETHEADERS because the only headers they could have would belong to a chain with enough work. There isn't a risk anymore of sending a bogus chain to a peer and getting disconnected.

This behavior was first introduced in https://gith
...

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2590020502)
I found myself confused by e8b589ef45d5c94c47209fb952e8b05d631a9c47 dropping `AddSocketPermissionFlags`. Can you move that to a separate (earlier?) commit?

💬 marcofleon commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914888912)
I used the same format as the `-upnp` option above. Should be removed soon so probably doesn't matter too much.

💬 marcofleon commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914890473)
Same thing here, just imitating the `-upnp` format above.

💬 EthanHeilman commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2590077704)
> is this in draft for a reason?

@instagibbs Yes, my todos to make this not a draft are:

[ ] - Better PR description
[ ] - A few extra tapscript tests (it was most cat tapscript tests before)
[ ] - Doublechecking my code to make sure the rebase didn't break anything

I got most of this done over the weekend, I just haven't pushed it yet. Will likely push it tonight, Wednesday

👍 rkrux approved a pull request: "test: add coverage for unknown address type for `createwalletdescriptor`"
(https://github.com/bitcoin/bitcoin/pull/31635#pullrequestreview-2549914526)
tACK 4da7bfdcc902f22239e55cd9b90abf36b29edfde

Pretty straightforward change.

💬 maflcko commented on pull request "test: add coverage for unknown address type for `createwalletdescriptor`":
(https://github.com/bitcoin/bitcoin/pull/31635#issuecomment-2590114046)
lgtm ACK 4da7bfdcc902f22239e55cd9b90abf36b29edfde

💬 sipa commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1914957018)
The only way I can see this still being a problem is:
* Someone started a node with 23.x or earlier (before #25717)
* Started being fed a low-work headers chain.
* Upgraded to 30.x(?) or later (with this PR)
* Never made any headers sync progress since.

That seems pretty unlikely, so I think it's fine to remove this if/when we get rid of checkpoints.

💬 rkrux commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1914970351)
Doesn't `CMake` dependency have a "version used"?

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2590182291)
> Alternatively we could make -coinbasereservedweight refer only to the coinbase, but internally account for these other bytes. That's probably less confusing. We'll end up with a default a bit under 8000 though for simplicity we could keep the round number.

This change will alter the behavior, resulting in a transactions weight of `3992000 - 332 = 3991668` after calling GBT.

I think I prefer the first idea, which is similar to what @0xB10C suggested: renaming coinbasereservedweight to `-b
...

👍 hodlinator approved a pull request: "fuzz: Abort when global PRNG is used before SeedRand::ZEROS"
(https://github.com/bitcoin/bitcoin/pull/31548#pullrequestreview-2550045227)
re-ACK fa3c787b62af6abaac35a8f0d785becdb8871cc0

Switched 2 `inline` functions to `static` which more clearly communicates the purpose being translation-unit-locality.

💬 hodlinator commented on pull request "net: Disconnect message follow-ups to #28521":
(https://github.com/bitcoin/bitcoin/pull/31633#discussion_r1915024508)
What do you think about downgrading it to `LogTrace`?

💬 hodlinator commented on pull request "net: Disconnect message follow-ups to #28521":
(https://github.com/bitcoin/bitcoin/pull/31633#discussion_r1915026583)
Agree, will change if I re-touch.

💬 hodlinator commented on pull request "net: Disconnect message follow-ups to #28521":
(https://github.com/bitcoin/bitcoin/pull/31633#discussion_r1915037547)
ping on my main system seems to care about ms+3 decimals (I'm guessing that's milliseconds+3 decimals => microseconds):
```
₿ ping 192.168.1.133
PING 192.168.1.133 (192.168.1.133): 56 data bytes
64 bytes from 192.168.1.133: icmp_seq=0 ttl=214 time=0.765 ms
```

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915042087)
re: https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1913589077

I didn't know about `digits()` and agree that it would be better to use than `sizeof()` so thanks for suggesting that.

> For my understanding, if it's not too much off-topic, I'd be curious to hear where the footgun potential is - since the point of my approach was to exactly avoid that?

My concern with using different types for input and output is that code like `auto kbps = SaturatingLeftShift(mbps, 10)` would be
...

💬 hodlinator commented on pull request "net: Disconnect message follow-ups to #28521":
(https://github.com/bitcoin/bitcoin/pull/31633#discussion_r1915045370)
I feel a slight itch here as well, but will probably leave casing as-is as in keeping with the original pull request.