💬 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

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1915015927)
4c23aacc2b82083f96255963f543dc35fff28334: since you need to print log statements here in a few places anyway, might as well drop the `errmsg` argument and always print logs here.

💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915078874)
> but that is what the current implementation in [e0a5417](https://github.com/bitcoin/bitcoin/commit/e0a541702def3a4c6cce8e44b6ebe8d608edad4e)) would do.

Are you sure it would? I don't see how the `Output` return type could be deduced with `auto`. That's kind of the point of the separate `Input` and `Output` types: it force the user to specify the return type, because especially with left shift the chances of the return type being much bigger than the input are high. Using (and potentially wr
...

💬 hebasto commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1915084448)
CMake 3.24.2 is used for release binaries in the Guix environment.