💬 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.

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1914992602)
4c23aacc2b82083f96255963f543dc35fff28334: I'm inclined to say "listening **on** %s"

As in: we bind _to_ a port in order to listen _on_ it.

The linux manual for `listen` also uses "on", as in "listen for connections on a socket"
https://man7.org/linux/man-pages/man2/listen.2.html