👍 ryanofsky approved a pull request: "ci: Update Clang in "tidy" job"
(https://github.com/bitcoin/bitcoin/pull/33445#pullrequestreview-3411868329)
Code review ACK 5d784bebaff5e3acc0b5180ee51d9a16aec0e356, just adding clang version comment since last review.
Would still be nice to remove ipc build code from the test directory as a followup too (https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2379651340)
(https://github.com/bitcoin/bitcoin/pull/33445#pullrequestreview-3411868329)
Code review ACK 5d784bebaff5e3acc0b5180ee51d9a16aec0e356, just adding clang version comment since last review.
Would still be nice to remove ipc build code from the test directory as a followup too (https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2379651340)
💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3481418578)
> But if sizes are serialized as `uint32_t`, I assume only the value range of `uint32_t` is valid, even on 64bit machines. In what scenario can `size_t` lead to a problem?
It is explained in the CVE (linked in the description):
"Before writing a block to disk, Bitcoin Core checks that its size is within a normal range. This check would overflow on 32-bit systems for blocks over 1GB..."
The basic idea is that the following code is brittle:
```
u32 value = result of some calculation,
...
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3481418578)
> But if sizes are serialized as `uint32_t`, I assume only the value range of `uint32_t` is valid, even on 64bit machines. In what scenario can `size_t` lead to a problem?
It is explained in the CVE (linked in the description):
"Before writing a block to disk, Bitcoin Core checks that its size is within a normal range. This check would overflow on 32-bit systems for blocks over 1GB..."
The basic idea is that the following code is brittle:
```
u32 value = result of some calculation,
...
👍 hebasto approved a pull request: "Add createwallet, createwalletdescriptor, and migratewallet to history filter"
(https://github.com/bitcoin-core/gui/pull/901#pullrequestreview-3411906243)
ACK 4e352efa2ce756c668664486c99d003eef530e0c.
(https://github.com/bitcoin-core/gui/pull/901#pullrequestreview-3411906243)
ACK 4e352efa2ce756c668664486c99d003eef530e0c.
🚀 hebasto merged a pull request: "Add createwallet, createwalletdescriptor, and migratewallet to history filter"
(https://github.com/bitcoin-core/gui/pull/901)
(https://github.com/bitcoin-core/gui/pull/901)
🤔 hodlinator reviewed a pull request: "log: reduce excessive "rolling back/forward" messages during block replay"
(https://github.com/bitcoin/bitcoin/pull/33443#pullrequestreview-3411788483)
Concept ACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596
(https://github.com/bitcoin/bitcoin/pull/33443#pullrequestreview-3411788483)
Concept ACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596
💬 hodlinator commented on pull request "log: reduce excessive "rolling back/forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2487048889)
In the commit message + PR description, what purpose does the `[*] `-prefix from the Before-example serve?
nit: Should uppercase "ibd" in PR desc.
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2487048889)
In the commit message + PR description, what purpose does the `[*] `-prefix from the Before-example serve?
nit: Should uppercase "ibd" in PR desc.
💬 hebasto commented on pull request "[30.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33609#issuecomment-3481469144)
Add https://github.com/bitcoin-core/gui/pull/901?
(https://github.com/bitcoin/bitcoin/pull/33609#issuecomment-3481469144)
Add https://github.com/bitcoin-core/gui/pull/901?
💬 purpleKarrot commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3481472896)
@maflcko, I get that checking for overflow after the cause is not going to work.
But if we have a buggy calculation that may overflow u32, changing the type to u64 is not a proper fix. What if it overflows u64? How do you want to detect that?
Would you agree that, if the calculation is fixed to detect overflow before the cause then using `size_t` would not be a problem at all?
(https://github.com/bitcoin/bitcoin/pull/33724#issuecomment-3481472896)
@maflcko, I get that checking for overflow after the cause is not going to work.
But if we have a buggy calculation that may overflow u32, changing the type to u64 is not a proper fix. What if it overflows u64? How do you want to detect that?
Would you agree that, if the calculation is fixed to detect overflow before the cause then using `size_t` would not be a problem at all?
💬 hebasto commented on pull request "Modernize custom filtering":
(https://github.com/bitcoin-core/gui/pull/899#discussion_r2487168803)
Thanks! Fixed.
(https://github.com/bitcoin-core/gui/pull/899#discussion_r2487168803)
Thanks! Fixed.
💬 stickies-v commented on pull request "log: reduce excessive "rolling back/forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2487174964)
http://github.com/bitcoin/bitcoin/pull/32604/commits/d541409a64c60d127ff912dad9dea949d45dbd8c#diff-44fd50b51e8fc6799d38f193237fb921ec9d34306c448f64837524a17bac06eeR471-R472 introduced prefixing all messages with `[*]` to indicate that ratelimiting is currently being applied.
(https://github.com/bitcoin/bitcoin/pull/33443#discussion_r2487174964)
http://github.com/bitcoin/bitcoin/pull/32604/commits/d541409a64c60d127ff912dad9dea949d45dbd8c#diff-44fd50b51e8fc6799d38f193237fb921ec9d34306c448f64837524a17bac06eeR471-R472 introduced prefixing all messages with `[*]` to indicate that ratelimiting is currently being applied.
💬 hebasto commented on pull request "Remove HD seed reference from blank wallet tooltip":
(https://github.com/bitcoin-core/gui/pull/908#issuecomment-3481506281)
cc @achow101
(https://github.com/bitcoin-core/gui/pull/908#issuecomment-3481506281)
cc @achow101
💬 0xB10C commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2487213289)
I plan have a closer look.
I don't use the utxocache tracepoints at the moment.
(https://github.com/bitcoin/bitcoin/pull/33680#discussion_r2487213289)
I plan have a closer look.
I don't use the utxocache tracepoints at the moment.
💬 yancyribbens commented on pull request "docs: add doc comment for SRD selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2487224418)
> SRD does NOT minimize the number of UTXOs included in the result. It only ensures that the selection adheres to the weight limit. Minimizing would imply that the selection is further pruned beyond adhering to the weight limit.
Your edit's make sense, thanks for the clarification. However, I thought it would be helpful to note _why_ the OutputGroups with the lowest `effective_value` are removed when the weight is exceeded. This seems at first counter-intuitive since it's not removing Outp
...
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2487224418)
> SRD does NOT minimize the number of UTXOs included in the result. It only ensures that the selection adheres to the weight limit. Minimizing would imply that the selection is further pruned beyond adhering to the weight limit.
Your edit's make sense, thanks for the clarification. However, I thought it would be helpful to note _why_ the OutputGroups with the lowest `effective_value` are removed when the weight is exceeded. This seems at first counter-intuitive since it's not removing Outp
...
👍 hebasto approved a pull request: "ci: Fix lint runner selection (and docker cache)"
(https://github.com/bitcoin/bitcoin/pull/33744#pullrequestreview-3412060705)
ACK 7632e0ba312a372259897c68fd7c7eb723df3738.
(https://github.com/bitcoin/bitcoin/pull/33744#pullrequestreview-3412060705)
ACK 7632e0ba312a372259897c68fd7c7eb723df3738.
💬 hebasto commented on pull request "ci: Fix lint runner selection (and docker cache)":
(https://github.com/bitcoin/bitcoin/pull/33744#discussion_r2487242811)
The GHA [docs](https://docs.github.com/en/actions/reference/workflows-and-actions/metadata-syntax) do not mention `inputs.<input_id>.options` at all.
(https://github.com/bitcoin/bitcoin/pull/33744#discussion_r2487242811)
The GHA [docs](https://docs.github.com/en/actions/reference/workflows-and-actions/metadata-syntax) do not mention `inputs.<input_id>.options` at all.
👍 hebasto approved a pull request: "random: scope environ extern to macOS, BSDs and Illumos"
(https://github.com/bitcoin/bitcoin/pull/33714#pullrequestreview-3412082427)
re-ACK 79d6f458e2300e1f47b94467cda233e1c761f8be.
(https://github.com/bitcoin/bitcoin/pull/33714#pullrequestreview-3412082427)
re-ACK 79d6f458e2300e1f47b94467cda233e1c761f8be.
💬 willcl-ark commented on pull request "ci: Fix lint runner selection (and docker cache)":
(https://github.com/bitcoin/bitcoin/pull/33744#discussion_r2487261226)
Yep, I think the original implementation got wires crossed with workflow dispatch inputs https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#example-of-onworkflow_dispatchinputs
(https://github.com/bitcoin/bitcoin/pull/33744#discussion_r2487261226)
Yep, I think the original implementation got wires crossed with workflow dispatch inputs https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#example-of-onworkflow_dispatchinputs
💬 yancyribbens commented on pull request "docs: add doc comment for SRD selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2487266295)
This edit is a tautology. No matter what the change budgeting is, the change amount will be at least `CHANGE_LOWER` since this value is added to `CHANGE_LOWER`. Maybe it's not the place to explain to users of the API _why_ it matters that `CHANGE_LOWER` may not be enough of a change budget, although I would have no idea why without reading the commit that added this parameter.
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2487266295)
This edit is a tautology. No matter what the change budgeting is, the change amount will be at least `CHANGE_LOWER` since this value is added to `CHANGE_LOWER`. Maybe it's not the place to explain to users of the API _why_ it matters that `CHANGE_LOWER` may not be enough of a change budget, although I would have no idea why without reading the commit that added this parameter.
💬 hebasto commented on issue "Async Payjoin":
(https://github.com/bitcoin/bitcoin/issues/33684#issuecomment-3481668679)
> Given that this is adding new features to the GUI. Is the plan to add them to the "legacy" GUI, the new QML GUI, or implement Payjoin into both? cc [@hebasto](https://github.com/hebasto)
cc @GBKS @johnny9
(https://github.com/bitcoin/bitcoin/issues/33684#issuecomment-3481668679)
> Given that this is adding new features to the GUI. Is the plan to add them to the "legacy" GUI, the new QML GUI, or implement Payjoin into both? cc [@hebasto](https://github.com/hebasto)
cc @GBKS @johnny9
💬 fanquake commented on pull request "feat: shell completions for bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/33747#issuecomment-3481668738)
Thanks. However aside from the fish addition, this duplicates works already being done in #33385 and #33402.
(https://github.com/bitcoin/bitcoin/pull/33747#issuecomment-3481668738)
Thanks. However aside from the fish addition, this duplicates works already being done in #33385 and #33402.