👍 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.
(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
(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.
(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"?
(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
...
(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.
(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`?
(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.
(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
```
(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
...
(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.
(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
(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.
(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
...
(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.
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1915084448)
CMake 3.24.2 is used for release binaries in the Guix environment.
🤔 jonatack reviewed a pull request: "test: avoid internet traffic"
(https://github.com/bitcoin/bitcoin/pull/31646#pullrequestreview-2550171291)
ACK 2ed161c5ce648cb66ec3d2941b02d68b6ca4c390
(did not observe an increase in speed on arm64 m1 max macos 15.2)
(https://github.com/bitcoin/bitcoin/pull/31646#pullrequestreview-2550171291)
ACK 2ed161c5ce648cb66ec3d2941b02d68b6ca4c390
(did not observe an increase in speed on arm64 m1 max macos 15.2)
🤔 mzumsande reviewed a pull request: "consensus: Remove checkpoints (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2550175831)
In #25725 the approach was to remove just the actual checkpoints, but leave the supporting code to be removed in the next / a later release (https://github.com/bitcoin/bitcoin/pull/25725#discussion_r1103582363). Did you consider this, instead of doing it all in one commit?
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2550175831)
In #25725 the approach was to remove just the actual checkpoints, but leave the supporting code to be removed in the next / a later release (https://github.com/bitcoin/bitcoin/pull/25725#discussion_r1103582363). Did you consider this, instead of doing it all in one commit?
👍 dergoegge approved a pull request: "consensus: Remove checkpoints (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2550177263)
ACK 9e91e536a45e3fe802eb8b38ccc893ef894e72e7
The "new" headers pre-sync (shipped in v24.0) protects us against the same denial-of-service issues that the checkpoints were solving in the past. Therefore it is reasonable to remove them, especially considering the testing the pre-sync has undergone and the amount of time that has passed.
Even under the assumption that there is a bug in the headers pre-sync logic (i.e. it's somehow still possible to submit low work headers), the checkpoints on
...
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2550177263)
ACK 9e91e536a45e3fe802eb8b38ccc893ef894e72e7
The "new" headers pre-sync (shipped in v24.0) protects us against the same denial-of-service issues that the checkpoints were solving in the past. Therefore it is reasonable to remove them, especially considering the testing the pre-sync has undergone and the amount of time that has passed.
Even under the assumption that there is a bug in the headers pre-sync logic (i.e. it's somehow still possible to submit low work headers), the checkpoints on
...
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2590320921)
If we're going to set a minimum value for `-blockreservedweight` we should probably just pick the absolute minimum:
- a block with only a coinbase transaction
- header weight + 1 (for the number of transactions)
- the coinbase has 1 input (required by `IsCoinBase()`)
- just the BIP34 scriptSig
- no witness commitment
- the coinbase has 1 output (required by `CheckTransaction`)
- `OP_TRUE` output script (or empty?)
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2590320921)
If we're going to set a minimum value for `-blockreservedweight` we should probably just pick the absolute minimum:
- a block with only a coinbase transaction
- header weight + 1 (for the number of transactions)
- the coinbase has 1 input (required by `IsCoinBase()`)
- just the BIP34 scriptSig
- no witness commitment
- the coinbase has 1 output (required by `CheckTransaction`)
- `OP_TRUE` output script (or empty?)
💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915119234)
> Are you sure it would? I don't see how the `Output` return type could be deduced with `auto`.
You're right. I didn't realize e0a541702def3a4c6cce8e44b6ebe8d608edad4e required specifying what type to cast the output to. In that case, I take back the comment that it has a footgun.
But I would say the implementation does not seem consistent with other functions in this file, or consistent with what I would expect a function that is doing a saturating multiplication operation to do. I think
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915119234)
> Are you sure it would? I don't see how the `Output` return type could be deduced with `auto`.
You're right. I didn't realize e0a541702def3a4c6cce8e44b6ebe8d608edad4e required specifying what type to cast the output to. In that case, I take back the comment that it has a footgun.
But I would say the implementation does not seem consistent with other functions in this file, or consistent with what I would expect a function that is doing a saturating multiplication operation to do. I think
...