💬 maflcko commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#discussion_r2006146926)
I just mean that the assert gives a hard upper bound, but the assume is a bit weaker on release builds, so if the code is used in a place where the upper bound is too small, the assume-version would continue to work on machines with enough memory and only crash "when needed" (aka when running oom).
(https://github.com/bitcoin/bitcoin/pull/31551#discussion_r2006146926)
I just mean that the assert gives a hard upper bound, but the assume is a bit weaker on release builds, so if the code is used in a place where the upper bound is too small, the assume-version would continue to work on machines with enough memory and only crash "when needed" (aka when running oom).
💬 vasild commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#issuecomment-2741214252)
`db442f3d6e...626cc06c69`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/29418#issuecomment-2741214252)
`db442f3d6e...626cc06c69`: rebase due to conflicts
💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2741231347)
`8c0ce1ca1c...dcb0be18ac`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2741231347)
`8c0ce1ca1c...dcb0be18ac`: rebase due to conflicts
💬 murchandamus commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2741233287)
The `long_term_fee_rate` and `fee_rate` are the same for all UTXOs in a single coin selection attempt, so if two UTXOs have the same fee, their inputs have the same weight, if they also have the same effective value, they are interchangeable. If they only have the same effective value but different fees, it means that they have differing weights. Similarly, waste simply scales with the weight across UTXOs, as it is weight times `long_term_fee_rate - fee_rate`. So, if two UTXOs have the same effe
...
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2741233287)
The `long_term_fee_rate` and `fee_rate` are the same for all UTXOs in a single coin selection attempt, so if two UTXOs have the same fee, their inputs have the same weight, if they also have the same effective value, they are interchangeable. If they only have the same effective value but different fees, it means that they have differing weights. Similarly, waste simply scales with the weight across UTXOs, as it is weight times `long_term_fee_rate - fee_rate`. So, if two UTXOs have the same effe
...
🤔 Jassor909 reviewed a pull request: "refactor: Simplifies ProcessMessage for NetMsgType::TX"
(https://github.com/bitcoin/bitcoin/pull/32104#pullrequestreview-2703636828)
@DrahtBot
(https://github.com/bitcoin/bitcoin/pull/32104#pullrequestreview-2703636828)
@DrahtBot
💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2741259247)
`72ff6d2b50...696b6671da`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2741259247)
`72ff6d2b50...696b6671da`: rebase due to conflicts
🤔 BrandonOdiwuor reviewed a pull request: "test: avoid treating hash results as integers"
(https://github.com/bitcoin/bitcoin/pull/32050#pullrequestreview-2703676924)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32050#pullrequestreview-2703676924)
Concept ACK
💬 sr-gi commented on pull request "refactor: Enforces Txid and Wtxid types in RelayTransaction":
(https://github.com/bitcoin/bitcoin/pull/32104#discussion_r2006210663)
I've updated the approach to enforce RelayTransaction types, and extended it to all other calls to the method
(https://github.com/bitcoin/bitcoin/pull/32104#discussion_r2006210663)
I've updated the approach to enforce RelayTransaction types, and extended it to all other calls to the method
💬 yancyribbens commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2006228794)
I tested however the benchmarks don't seem to be sensitive enough for this to make a difference.
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2006228794)
I tested however the benchmarks don't seem to be sensitive enough for this to make a difference.
🤔 Jassor909 reviewed a pull request: "refactor: Enforces Txid and Wtxid types in RelayTransaction"
(https://github.com/bitcoin/bitcoin/pull/32104#pullrequestreview-2703737664)
#
(https://github.com/bitcoin/bitcoin/pull/32104#pullrequestreview-2703737664)
#
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2006247029)
... or a poor design of Qt's internal find modules.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2006247029)
... or a poor design of Qt's internal find modules.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2741372636)
My Guix build:
```
aarch64
8948298e720d0d9077a1f57c6e44fff86a234830547f5d57278d37c672236541 guix-build-7b51bf1d4c19/output/aarch64-linux-gnu/SHA256SUMS.part
43cec316ed0a89274e9b805b845e5be6b164b9ba5b58750fee470335ed4f7904 guix-build-7b51bf1d4c19/output/aarch64-linux-gnu/bitcoin-7b51bf1d4c19-aarch64-linux-gnu-debug.tar.gz
023042d946d06d6a89e5a611bd5eb9e73680c541e676e4b4c0c28ea780e9fc71 guix-build-7b51bf1d4c19/output/aarch64-linux-gnu/bitcoin-7b51bf1d4c19-aarch64-linux-gnu.tar.gz
513e04e1
...
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2741372636)
My Guix build:
```
aarch64
8948298e720d0d9077a1f57c6e44fff86a234830547f5d57278d37c672236541 guix-build-7b51bf1d4c19/output/aarch64-linux-gnu/SHA256SUMS.part
43cec316ed0a89274e9b805b845e5be6b164b9ba5b58750fee470335ed4f7904 guix-build-7b51bf1d4c19/output/aarch64-linux-gnu/bitcoin-7b51bf1d4c19-aarch64-linux-gnu-debug.tar.gz
023042d946d06d6a89e5a611bd5eb9e73680c541e676e4b4c0c28ea780e9fc71 guix-build-7b51bf1d4c19/output/aarch64-linux-gnu/bitcoin-7b51bf1d4c19-aarch64-linux-gnu.tar.gz
513e04e1
...
💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2741407835)
re-ACK 418236c106e32abd7357551d309f8e6d1e494f72
Same as my last review, but the two commits shared with #31866 are merged.
Also tested (ad hoc signed) guix builds on macOS (M4 with 15.3.2, Intel with 13.7.4).
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2741407835)
re-ACK 418236c106e32abd7357551d309f8e6d1e494f72
Same as my last review, but the two commits shared with #31866 are merged.
Also tested (ad hoc signed) guix builds on macOS (M4 with 15.3.2, Intel with 13.7.4).
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006272875)
The [latest force-push](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_29..kernelApi_30) broke this logic by leaving `m_chainparams` and `m_notifications` uninitialized if `options` is non-nullptr, but the respective options members are nullptr.
Suggested fix:
<details>
<summary>git diff on 2dc27e2860</summary>
```diff
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index 0cb2d69cec..1e6c582357 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006272875)
The [latest force-push](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_29..kernelApi_30) broke this logic by leaving `m_chainparams` and `m_notifications` uninitialized if `options` is non-nullptr, but the respective options members are nullptr.
Suggested fix:
<details>
<summary>git diff on 2dc27e2860</summary>
```diff
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index 0cb2d69cec..1e6c582357 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/
...
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2000864918)
typo nit (+ in 2 other log functions)
```suggestion
* all existing `kernel_LoggingConnection instances.
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2000864918)
typo nit (+ in 2 other log functions)
```suggestion
* all existing `kernel_LoggingConnection instances.
```
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006290487)
What is the rationale behind requiring to first add, and then {enable,disable} the category? An alternative would be a single `kernel_set_log_level_category`, which immediately "enables" (I think it's a strange term anyway) the category at the given level. "Disabling" would be achieved by calling `kernel_set_log_level_category` again with a higher (i.e. less granular) level, again taking effect immediately.
I can't think of any use cases that require separating this in 3 functions? I think it
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006290487)
What is the rationale behind requiring to first add, and then {enable,disable} the category? An alternative would be a single `kernel_set_log_level_category`, which immediately "enables" (I think it's a strange term anyway) the category at the given level. "Disabling" would be achieved by calling `kernel_set_log_level_category` again with a higher (i.e. less granular) level, again taking effect immediately.
I can't think of any use cases that require separating this in 3 functions? I think it
...
💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006309594)
> I don't think we need to rely on the order inside the enumeration here
I don't think it's required, but it is slightly convenient when they are? E.g. when implementing `py-bitcoinkernel`'s logging, being able to rely on the order of `kernel_LogLevel` helps the implementation a bit (and it is also how these level enums are usually implemented in most logging libraries, I think). As one example, I think using the same values as the python `logging` library could be sensible: https://docs.pyth
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006309594)
> I don't think we need to rely on the order inside the enumeration here
I don't think it's required, but it is slightly convenient when they are? E.g. when implementing `py-bitcoinkernel`'s logging, being able to rely on the order of `kernel_LogLevel` helps the implementation a bit (and it is also how these level enums are usually implemented in most logging libraries, I think). As one example, I think using the same values as the python `logging` library could be sensible: https://docs.pyth
...
💬 willcl-ark commented on issue "b-msghand invoked oom-killer: Master (v28.99) crashing during IBD":
(https://github.com/bitcoin/bitcoin/issues/31561#issuecomment-2741665911)
I may take a stab at trying to reproduce this to see if it's still a problem with master today (unless you fancy doing so).
Just a few questions first:
> Configuration is just -txindex=1
- Their (basic) 8GB droplets appear to have 160 GiB (SSD), so I presume you also had pruning on?
- Your logs show at least `bench` and `validation` debug levels, did you use `-debug=1`, or only these (or only these on a subsequent run to debug a crash)?
- Am I likely to need to sync to ~ `height=814565` to se
...
(https://github.com/bitcoin/bitcoin/issues/31561#issuecomment-2741665911)
I may take a stab at trying to reproduce this to see if it's still a problem with master today (unless you fancy doing so).
Just a few questions first:
> Configuration is just -txindex=1
- Their (basic) 8GB droplets appear to have 160 GiB (SSD), so I presume you also had pruning on?
- Your logs show at least `bench` and `validation` debug levels, did you use `-debug=1`, or only these (or only these on a subsequent run to debug a crash)?
- Am I likely to need to sync to ~ `height=814565` to se
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006471733)
Thanks! I also added a regression test. Sorry for not catching this earlier!
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2006471733)
Thanks! I also added a regression test. Sorry for not catching this earlier!
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2741688322)
Updated 2dc27e2860b97c2bffa5f18706917b21858e5594 -> 9fc6accf89ed001f70e107a8e9936f6dc3a35f41 ([kernelApi_30](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_30) -> [kernelApi_31](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_31), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_30..kernelApi_31))
* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2000864918), fixed naming in docstring for `kernel_LoggingConnection`.
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2741688322)
Updated 2dc27e2860b97c2bffa5f18706917b21858e5594 -> 9fc6accf89ed001f70e107a8e9936f6dc3a35f41 ([kernelApi_30](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_30) -> [kernelApi_31](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_31), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_30..kernelApi_31))
* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2000864918), fixed naming in docstring for `kernel_LoggingConnection`.
...