👍 TheCharlatan approved a pull request: "init,log: Unify block index log line"
(https://github.com/bitcoin/bitcoin/pull/31616#pullrequestreview-2541869126)
ACK e04be3731f4921cd51d25b1d6210eace7600fea4
(https://github.com/bitcoin/bitcoin/pull/31616#pullrequestreview-2541869126)
ACK e04be3731f4921cd51d25b1d6210eace7600fea4
💬 maflcko commented on pull request "qa: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1910044619)
coming from the conflicting pull, I don't think this call should exist in the first place for two reasons:
* It doesn't work in most cases, because a failure to start any node likely means the RPC isn't up for the other nodes either, so the call will just fail in most cases. Effectively just using the kill fallback.
* It doesn't make sense to special case startup. Nodes can at any time fail to start (not only in start_nodes), or crash, and adding this exception handling to all places isn't u
...
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1910044619)
coming from the conflicting pull, I don't think this call should exist in the first place for two reasons:
* It doesn't work in most cases, because a failure to start any node likely means the RPC isn't up for the other nodes either, so the call will just fail in most cases. Effectively just using the kill fallback.
* It doesn't make sense to special case startup. Nodes can at any time fail to start (not only in start_nodes), or crash, and adding this exception handling to all places isn't u
...
💬 hebasto commented on pull request "depends: Use base system's `sha256sum` utility on FreeBSD":
(https://github.com/bitcoin/bitcoin/pull/31626#issuecomment-2582141098)
> Could add freebsd to the title now?
Added.
(https://github.com/bitcoin/bitcoin/pull/31626#issuecomment-2582141098)
> Could add freebsd to the title now?
Added.
💬 hebasto commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#discussion_r1910054910)
I suggest to limit the scope of this warning suppression to the `bitcoin_crypto` library only:
```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index f2a8183c84..2dba6f255d 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -397,7 +397,6 @@ target_link_libraries(core_interface INTERFACE warn_interface)
if(MSVC)
try_append_cxx_flags("/W3" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("/wd4018" TARGET warn_interface SKIP_LINK)
- try_append_cxx_flags("/wd4146" TARGET
...
(https://github.com/bitcoin/bitcoin/pull/21590#discussion_r1910054910)
I suggest to limit the scope of this warning suppression to the `bitcoin_crypto` library only:
```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index f2a8183c84..2dba6f255d 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -397,7 +397,6 @@ target_link_libraries(core_interface INTERFACE warn_interface)
if(MSVC)
try_append_cxx_flags("/W3" TARGET warn_interface SKIP_LINK)
try_append_cxx_flags("/wd4018" TARGET warn_interface SKIP_LINK)
- try_append_cxx_flags("/wd4146" TARGET
...
💬 Sjors commented on pull request "miner: always treat SegWit as active":
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2582176125)
It would be good if someone has a link to the earlier reasoning.
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2582176125)
It would be good if someone has a link to the earlier reasoning.
💬 l0rinc commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2582176601)
Thank you @Sjors for testing it.
I was surprised to see your config only revealed a 3% change so I reran the full IBDs with the configs you had: `-dbcache=30000 -stopatheight=878000` (I had `-dbcache=1000 -stopatheight=870000` before).
I suspect the difference in our measurements could stem from doing a single run and not including the final dump in the measurements.
I wasn't seeding from local nodes, so the variance was a bit bigger for me, but I ran both before and after several times an
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2582176601)
Thank you @Sjors for testing it.
I was surprised to see your config only revealed a 3% change so I reran the full IBDs with the configs you had: `-dbcache=30000 -stopatheight=878000` (I had `-dbcache=1000 -stopatheight=870000` before).
I suspect the difference in our measurements could stem from doing a single run and not including the final dump in the measurements.
I wasn't seeding from local nodes, so the variance was a bit bigger for me, but I ran both before and after several times an
...
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910076808)
> I think the `weight` and `sigops` results of `getblocktemplate` are for individual transactions in the block template.
Ah that would make sense, since I tried to add those totals in an early version of #27433.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910076808)
> I think the `weight` and `sigops` results of `getblocktemplate` are for individual transactions in the block template.
Ah that would make sense, since I tried to add those totals in an early version of #27433.
💬 maflcko commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2582264739)
Not sure how useful it is to derive speed improvements from measurements where the variance is about half as large as the difference itself. Not claiming this is the case here, but if you measure from the public network, you could very well just measure the bandwidth of the picked nodes (completely unrelated to this pull).
It is fine if you want to do those measurements locally for fun, but putting them in the pull request title and description doesn't seem ideal. It would be better to focus
...
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2582264739)
Not sure how useful it is to derive speed improvements from measurements where the variance is about half as large as the difference itself. Not claiming this is the case here, but if you measure from the public network, you could very well just measure the bandwidth of the picked nodes (completely unrelated to this pull).
It is fine if you want to do those measurements locally for fun, but putting them in the pull request title and description doesn't seem ideal. It would be better to focus
...
👍 Sjors approved a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2541983102)
ACK 7ab1344eb13228998bcead7328d5cc0a905971d4
Just some feedback on the release notes.
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2541983102)
ACK 7ab1344eb13228998bcead7328d5cc0a905971d4
Just some feedback on the release notes.
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910102771)
7ab1344eb13228998bcead7328d5cc0a905971d4
`-blockmaxweight` was never `4,000,000` by default
Maybe change this and the next two lines to:
```md
When the node constructs a new block, e.g. for a `getblocktemplate` RPC call,
it doesn't generate the coinbase transaction. Instead it reserves `4,000` weight
units (WU) for it. Before this pull request, the default for `-blockmaxweight`
was `3,996,000`, which effectively added another `4,000` weight units (WU) to
this reservation, leading t
...
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910102771)
7ab1344eb13228998bcead7328d5cc0a905971d4
`-blockmaxweight` was never `4,000,000` by default
Maybe change this and the next two lines to:
```md
When the node constructs a new block, e.g. for a `getblocktemplate` RPC call,
it doesn't generate the coinbase transaction. Instead it reserves `4,000` weight
units (WU) for it. Before this pull request, the default for `-blockmaxweight`
was `3,996,000`, which effectively added another `4,000` weight units (WU) to
this reservation, leading t
...
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910120119)
I think think this wording is a bit too vague, how about:
```md
Users who manually set `-blockmaxweight` to its maximum value `4,000,000` can lower
`-coinbasereservedweight` to `4,000` to maintain the previous behaviour.
```
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910120119)
I think think this wording is a bit too vague, how about:
```md
Users who manually set `-blockmaxweight` to its maximum value `4,000,000` can lower
`-coinbasereservedweight` to `4,000` to maintain the previous behaviour.
```
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910123643)
`-coinbasereservedweight`
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910123643)
`-coinbasereservedweight`
💬 l0rinc commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2582274342)
Usually the variance is a lot lower (see previous measurements), but these are just my benchmarks (I want them to be as close to reality as possible, that's why I'm repeating them to have some predictability), I would appreciate if you could provide independent measurements that you find more stable.
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2582274342)
Usually the variance is a lot lower (see previous measurements), but these are just my benchmarks (I want them to be as close to reality as possible, that's why I'm repeating them to have some predictability), I would appreciate if you could provide independent measurements that you find more stable.
💬 hodlinator commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1910139282)
Agree it's a bit verbose right now. Especially like the first suggestion in the diff.
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1910139282)
Agree it's a bit verbose right now. Especially like the first suggestion in the diff.
👍 hodlinator approved a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2542035711)
re-ACK 82706e217f34d1f09cbd30dde6b8ae5ac0385f0a
Mixing of MiB and bytes in constants is tolerable, and at least the units are documented.
New unit tests look good to me.
---
Nit: commit message in 82706e217f34d1f09cbd30dde6b8ae5ac0385f0a contains 3 ways of describing the number of bits:
- "memory was allocated on 32bit platforms."
- "maximum size of a 32 bit unsigned"
- "in behaviour on 32-bit systems."
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2542035711)
re-ACK 82706e217f34d1f09cbd30dde6b8ae5ac0385f0a
Mixing of MiB and bytes in constants is tolerable, and at least the units are documented.
New unit tests look good to me.
---
Nit: commit message in 82706e217f34d1f09cbd30dde6b8ae5ac0385f0a contains 3 ways of describing the number of bits:
- "memory was allocated on 32bit platforms."
- "maximum size of a 32 bit unsigned"
- "in behaviour on 32-bit systems."
💬 maflcko commented on pull request "miner: always treat SegWit as active":
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2582320880)
Not sure if it was ever mentioned publicly where a link exists.
Obviously, if you want to reproduce the inconsistency, it would be easier in regtest, and the change may theoretically break someone's test deployment.
(https://github.com/bitcoin/bitcoin/pull/31625#issuecomment-2582320880)
Not sure if it was ever mentioned publicly where a link exists.
Obviously, if you want to reproduce the inconsistency, it would be easier in regtest, and the change may theoretically break someone's test deployment.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910175994)
Taken, thanks.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910175994)
Taken, thanks.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910176196)
fixed
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910176196)
fixed
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910177236)
fixed
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1910177236)
fixed
💬 0xB10C commented on pull request "tracing: Rename the `MIN` macro to `_TRACEPOINT_TEST_MIN` in log_raw_p2p_msgs":
(https://github.com/bitcoin/bitcoin/pull/31623#issuecomment-2582351026)
tested ACK f93f0c93961bbce413101c2a92300a7a29277506
(https://github.com/bitcoin/bitcoin/pull/31623#issuecomment-2582351026)
tested ACK f93f0c93961bbce413101c2a92300a7a29277506