💬 achow101 commented on pull request "test: Fix TestFlushBehavior when coin has non-zero dynamic memory usage":
(https://github.com/bitcoin/bitcoin/pull/33381#issuecomment-3304517512)
This change breaks the test.
(https://github.com/bitcoin/bitcoin/pull/33381#issuecomment-3304517512)
This change breaks the test.
💬 achow101 commented on pull request "rpc: Add validation for invalid taproot signatures in analyzepsbt":
(https://github.com/bitcoin/bitcoin/pull/33360#discussion_r2356755028)
This PSBT isn't serialized correctly.
(https://github.com/bitcoin/bitcoin/pull/33360#discussion_r2356755028)
This PSBT isn't serialized correctly.
💬 achow101 commented on pull request "doc: Add documentation explaining different build types":
(https://github.com/bitcoin/bitcoin/pull/33355#issuecomment-3304532974)
Please stop using LLMs to generate PRs.
(https://github.com/bitcoin/bitcoin/pull/33355#issuecomment-3304532974)
Please stop using LLMs to generate PRs.
✅ achow101 closed a pull request: "doc: Add documentation explaining different build types"
(https://github.com/bitcoin/bitcoin/pull/33355)
(https://github.com/bitcoin/bitcoin/pull/33355)
💬 l0rinc commented on pull request "index: remove unnecessary locator cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#issuecomment-3304577242)
ACK facd01e6ffbbd019312f370a3807de0b95bbd745
Note that we don't usually add the `Signed off by` to the commit message, it just seems like noise to me. You can still sign your commits if you want, but I'm no sure what this line adds that isn't already obvious.
Also, not sure why `refactor: remove redundant locator cleanup in BaseIndex::Init()` is repeated (slightly differently) in the PR description.
(https://github.com/bitcoin/bitcoin/pull/32882#issuecomment-3304577242)
ACK facd01e6ffbbd019312f370a3807de0b95bbd745
Note that we don't usually add the `Signed off by` to the commit message, it just seems like noise to me. You can still sign your commits if you want, but I'm no sure what this line adds that isn't already obvious.
Also, not sure why `refactor: remove redundant locator cleanup in BaseIndex::Init()` is repeated (slightly differently) in the PR description.
💬 achow101 commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2356816234)
In 65a10fc3c52ea09a4794345bcf607dff908c783a "p2p: add assertion for BlockTransactionsRequest indexes"
Can checking this be done in deserialization directly, rather than iterating all of the indexes again? Since this is part of compact block relay, we want to avoid doing as much unnecessary work as possible.
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2356816234)
In 65a10fc3c52ea09a4794345bcf607dff908c783a "p2p: add assertion for BlockTransactionsRequest indexes"
Can checking this be done in deserialization directly, rather than iterating all of the indexes again? Since this is part of compact block relay, we want to avoid doing as much unnecessary work as possible.
💬 achow101 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3304776460)
> because it used legacy wallet operations
That's not why it fails, it does not use anything specific to legacy wallets at all. It fails initially because test nodes are started with `-disablewallet` if `self.uses_wallet == False`, and then fails again because of multiwallet, and lastly because `settxfee` is deprecated. But none of these are because of legacy wallet.
Ultimately, the issues stem from the fact that this code path is pretty much never executed, and if MiniWallet were to chang
...
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3304776460)
> because it used legacy wallet operations
That's not why it fails, it does not use anything specific to legacy wallets at all. It fails initially because test nodes are started with `-disablewallet` if `self.uses_wallet == False`, and then fails again because of multiwallet, and lastly because `settxfee` is deprecated. But none of these are because of legacy wallet.
Ultimately, the issues stem from the fact that this code path is pretty much never executed, and if MiniWallet were to chang
...
💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2356963792)
In 9aaf55397159bb0e839c19d3d4578dfa144a059b "rpc: Handle -named argument parsing where '=' character is used"
This shouldn't be deleted.
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2356963792)
In 9aaf55397159bb0e839c19d3d4578dfa144a059b "rpc: Handle -named argument parsing where '=' character is used"
This shouldn't be deleted.
💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2356967273)
In 9aaf55397159bb0e839c19d3d4578dfa144a059b "rpc: Handle -named argument parsing where '=' character is used"
nit: space between `s` and `:`
```suggestion
for (std::string_view s : strParams) {
```
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2356967273)
In 9aaf55397159bb0e839c19d3d4578dfa144a059b "rpc: Handle -named argument parsing where '=' character is used"
nit: space between `s` and `:`
```suggestion
for (std::string_view s : strParams) {
```
💬 achow101 commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2356965832)
In 9aaf55397159bb0e839c19d3d4578dfa144a059b "rpc: Handle -named argument parsing where '=' character is used"
Extraneous newline
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2356965832)
In 9aaf55397159bb0e839c19d3d4578dfa144a059b "rpc: Handle -named argument parsing where '=' character is used"
Extraneous newline
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3304806063)
> > because it used legacy wallet operations
>
> That's not why it fails, it does not use anything specific to legacy wallets at all. It fails initially because test nodes are started with `-disablewallet` if `self.uses_wallet == False`, and then fails again because of multiwallet, and lastly because `settxfee` is deprecated. But none of these are because of legacy wallet.
>
> Ultimately, the issues stem from the fact that this code path is pretty much never executed, and if MiniWallet were to
...
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3304806063)
> > because it used legacy wallet operations
>
> That's not why it fails, it does not use anything specific to legacy wallets at all. It fails initially because test nodes are started with `-disablewallet` if `self.uses_wallet == False`, and then fails again because of multiwallet, and lastly because `settxfee` is deprecated. But none of these are because of legacy wallet.
>
> Ultimately, the issues stem from the fact that this code path is pretty much never executed, and if MiniWallet were to
...
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3304812339)
> > because it used legacy wallet operations
>
> That's not why it fails, it does not use anything specific to legacy wallets at all. It fails initially because test nodes are started with `-disablewallet` if `self.uses_wallet == False`, and then fails again because of multiwallet, and lastly because `settxfee` is deprecated. But none of these are because of legacy wallet.
>
> Ultimately, the issues stem from the fact that this code path is pretty much never executed, and if MiniWallet were to
...
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3304812339)
> > because it used legacy wallet operations
>
> That's not why it fails, it does not use anything specific to legacy wallets at all. It fails initially because test nodes are started with `-disablewallet` if `self.uses_wallet == False`, and then fails again because of multiwallet, and lastly because `settxfee` is deprecated. But none of these are because of legacy wallet.
>
> Ultimately, the issues stem from the fact that this code path is pretty much never executed, and if MiniWallet were to
...
💬 Raimo33 commented on pull request "Remove unnecessary casts when calling socket operations":
(https://github.com/bitcoin/bitcoin/pull/33378#issuecomment-3304898737)
ACK 67f632b6deb8b4aa190c458b71d2bc8c793626d5
(https://github.com/bitcoin/bitcoin/pull/33378#issuecomment-3304898737)
ACK 67f632b6deb8b4aa190c458b71d2bc8c793626d5
💬 luke-jr commented on pull request "cmake: Inherit WERROR setting for secp256k1 build":
(https://github.com/bitcoin/bitcoin/pull/33297#issuecomment-3305020767)
Concept ACK. As long as -DWERROR is supported, it should work correctly.
Discussion about dropping it in favour of other solutions should be considered separately (and existing branches should get fixed even if it's dropped in newer versions)
(https://github.com/bitcoin/bitcoin/pull/33297#issuecomment-3305020767)
Concept ACK. As long as -DWERROR is supported, it should work correctly.
Discussion about dropping it in favour of other solutions should be considered separately (and existing branches should get fixed even if it's dropped in newer versions)
👍 davidgumberg approved a pull request: "Remove unnecessary casts when calling socket operations"
(https://github.com/bitcoin/bitcoin/pull/33378#pullrequestreview-3236853659)
ACK https://github.com/bitcoin/bitcoin/commit/67f632b6deb8b4aa190c458b71d2bc8c793626d5
As described in the commit messages, the `Sock::(G|S)etSockOpt` functions are already casting correctly, so there shouldn't be casting when using those functions. The only other users of `(g|s)etsockopt` are the httpserver (which doesn't use `Sock`):
https://github.com/bitcoin/bitcoin/blob/67f632b6deb8b4aa190c458b71d2bc8c793626d5/src/httpserver.cpp#L404
and the socket unit tests:
https://github.com
...
(https://github.com/bitcoin/bitcoin/pull/33378#pullrequestreview-3236853659)
ACK https://github.com/bitcoin/bitcoin/commit/67f632b6deb8b4aa190c458b71d2bc8c793626d5
As described in the commit messages, the `Sock::(G|S)etSockOpt` functions are already casting correctly, so there shouldn't be casting when using those functions. The only other users of `(g|s)etsockopt` are the httpserver (which doesn't use `Sock`):
https://github.com/bitcoin/bitcoin/blob/67f632b6deb8b4aa190c458b71d2bc8c793626d5/src/httpserver.cpp#L404
and the socket unit tests:
https://github.com
...
💬 luke-jr commented on pull request "RFC: blocks: add `-reobfuscate-blocks` arg to xor existing blk/rev on startup":
(https://github.com/bitcoin/bitcoin/pull/33324#issuecomment-3305066838)
I agree a separate utility for this seems better - this requires very little of the existing codebase, in theory.
Also suggest making the files with the same names, but in a new directory, and then atomically rename the directory when complete, rather than every single file.
(https://github.com/bitcoin/bitcoin/pull/33324#issuecomment-3305066838)
I agree a separate utility for this seems better - this requires very little of the existing codebase, in theory.
Also suggest making the files with the same names, but in a new directory, and then atomically rename the directory when complete, rather than every single file.
💬 luke-jr commented on pull request "crypto: optimize SipHash Write() method with chunked processing":
(https://github.com/bitcoin/bitcoin/pull/33325#issuecomment-3305075347)
> I just realized this was attempted in the past. merged in Bitcoin knots 19. restored to match Core's implementation in knots 29. 🤔.
#18014 is still in Knots 29...
(https://github.com/bitcoin/bitcoin/pull/33325#issuecomment-3305075347)
> I just realized this was attempted in the past. merged in Bitcoin knots 19. restored to match Core's implementation in knots 29. 🤔.
#18014 is still in Knots 29...
💬 l0rinc commented on pull request "crypto: optimize SipHash Write() method with chunked processing":
(https://github.com/bitcoin/bitcoin/pull/33325#issuecomment-3305097747)
@luke-jr where is this general writer needed? We already have separate specialized siphash implementation for the critical usecases...
(https://github.com/bitcoin/bitcoin/pull/33325#issuecomment-3305097747)
@luke-jr where is this general writer needed? We already have separate specialized siphash implementation for the critical usecases...
💬 l0rinc commented on pull request "RFC: blocks: add `-reobfuscate-blocks` arg to xor existing blk/rev on startup":
(https://github.com/bitcoin/bitcoin/pull/33324#issuecomment-3305103297)
> I agree a separate utility for this seems better
Can you quote what you're agreeing with specifically, not sure who suggested that.
Besides, @andrewtoth already has a tool for that, it was mentioned in the PR description.
> but in a new directory, and then atomically rename
I will think about it, could make sense, but in that case unrelated files should also be copied over (maybe duplicated to be safe) - and listing the directory content wouldn't make the progress obvious. What's wr
...
(https://github.com/bitcoin/bitcoin/pull/33324#issuecomment-3305103297)
> I agree a separate utility for this seems better
Can you quote what you're agreeing with specifically, not sure who suggested that.
Besides, @andrewtoth already has a tool for that, it was mentioned in the PR description.
> but in a new directory, and then atomically rename
I will think about it, could make sense, but in that case unrelated files should also be copied over (maybe duplicated to be safe) - and listing the directory content wouldn't make the progress obvious. What's wr
...
💬 HowHsu commented on pull request "index: remove unnecessary locator cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#issuecomment-3305245694)
> ACK [facd01e](https://github.com/bitcoin/bitcoin/commit/facd01e6ffbbd019312f370a3807de0b95bbd745)
>
> Note that we don't usually add the `Signed off by` to the commit message, it just seems like noise to me. You can still sign your commits if you want, but I'm no sure what this line adds that isn't already obvious.
>
Added by my git config automatically, that is a kind of DCO for GPLv2, I'll remove it in later PRs since Bitcoin Core is MIT, so yes, it is not necessary to Sign.
> Als
...
(https://github.com/bitcoin/bitcoin/pull/32882#issuecomment-3305245694)
> ACK [facd01e](https://github.com/bitcoin/bitcoin/commit/facd01e6ffbbd019312f370a3807de0b95bbd745)
>
> Note that we don't usually add the `Signed off by` to the commit message, it just seems like noise to me. You can still sign your commits if you want, but I'm no sure what this line adds that isn't already obvious.
>
Added by my git config automatically, that is a kind of DCO for GPLv2, I'll remove it in later PRs since Bitcoin Core is MIT, so yes, it is not necessary to Sign.
> Als
...