💬 stickies-v commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-2878217156)
PSA: Bitcoin Core PR Review Club will cover this PR in its next meeting at 2025-05-14 at 17:00 UTC. See https://bitcoincore.reviews/32317 for notes, questions, and instructions on [how to join](https://bitcoincore.reviews/).
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-2878217156)
PSA: Bitcoin Core PR Review Club will cover this PR in its next meeting at 2025-05-14 at 17:00 UTC. See https://bitcoincore.reviews/32317 for notes, questions, and instructions on [how to join](https://bitcoincore.reviews/).
💬 jmatcho commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2878273731)
Concept NACK for changing an existing default and deprecating an existing feature that take control away from the people's nodes.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2878273731)
Concept NACK for changing an existing default and deprecating an existing feature that take control away from the people's nodes.
💬 luke-jr commented on pull request "docs: clarify RPC credentials security boundary":
(https://github.com/bitcoin/bitcoin/pull/32424#issuecomment-2878375720)
Seems to contradict the existence of `-rpcwhitelist`
(https://github.com/bitcoin/bitcoin/pull/32424#issuecomment-2878375720)
Seems to contradict the existence of `-rpcwhitelist`
💬 bigshiny90 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2878447923)
Concept NACK
there is no clear need for this change. Use cases expressed are minor, with only hope of future usage. Expressed use cases can be accommodated in a simpler way with a minor size change. If Core would like this op return change for the future, do the work and convince node runners to change their defaults. Arguments for block propagation speed and fee estimates, though technically sound, are potentially minor compared to forcing the default size to max. Slow this down and move to
...
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2878447923)
Concept NACK
there is no clear need for this change. Use cases expressed are minor, with only hope of future usage. Expressed use cases can be accommodated in a simpler way with a minor size change. If Core would like this op return change for the future, do the work and convince node runners to change their defaults. Arguments for block propagation speed and fee estimates, though technically sound, are potentially minor compared to forcing the default size to max. Slow this down and move to
...
💬 BitcoinMechanic commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2878452352)
Did the NACK/ACK bot break? Doesn't seem to be updating? @DrahtBot
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2878452352)
Did the NACK/ACK bot break? Doesn't seem to be updating? @DrahtBot
👋 ajtowns's pull request is ready for review: "ArgsManager: support subcommand-specific options"
(https://github.com/bitcoin/bitcoin/pull/28802)
(https://github.com/bitcoin/bitcoin/pull/28802)
💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-2878651168)
Rebased and undrafted
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-2878651168)
Rebased and undrafted
💬 maflcko commented on pull request "fuzz: Properly setup wallet in wallet_fees target":
(https://github.com/bitcoin/bitcoin/pull/32488#issuecomment-2878682301)
(side note: I would have expected that the fuzz stability was less, but https://oss-fuzz.com/fuzzer-stats?group_by=by-fuzzer&date_start=2025-04-14&date_end=2025-05-13&fuzzer=afl_bitcoin-core_wallet_fees&job=afl_asan_bitcoin-core&project=bitcoin-core claims it is 100%)
(https://github.com/bitcoin/bitcoin/pull/32488#issuecomment-2878682301)
(side note: I would have expected that the fuzz stability was less, but https://oss-fuzz.com/fuzzer-stats?group_by=by-fuzzer&date_start=2025-04-14&date_end=2025-05-13&fuzzer=afl_bitcoin-core_wallet_fees&job=afl_asan_bitcoin-core&project=bitcoin-core claims it is 100%)
💬 maflcko commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2878722476)
> Did the NACK/ACK bot break? Doesn't seem to be updating? @DrahtBot
This is a known issue. See https://github.com/maflcko/DrahtBot/issues/51. However, I can't spot the error, as explained in that thread.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2878722476)
> Did the NACK/ACK bot break? Doesn't seem to be updating? @DrahtBot
This is a known issue. See https://github.com/maflcko/DrahtBot/issues/51. However, I can't spot the error, as explained in that thread.
💬 maflcko commented on pull request "Fix ASM ambiguity":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-2878740702)
ping @willcl-ark. There hasn't been any activity here since the revived discussion more than a year ago.
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-2878740702)
ping @willcl-ark. There hasn't been any activity here since the revived discussion more than a year ago.
💬 chrisguida commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2878752046)
Concept NACK
I definitely already did this, but I don't see my comment anymore and it was never registered by drahtbot, so I'm redoing it.
Same rationale as https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2833932824
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2878752046)
Concept NACK
I definitely already did this, but I don't see my comment anymore and it was never registered by drahtbot, so I'm redoing it.
Same rationale as https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2833932824
💬 Eunovo commented on pull request "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2878821724)
> Concept NACK
>
> The purpose of the `private` argument is to show the private keys. If there are no private keys to show, I think it is a reasonable error to say that, rather than just showing the public descriptors. I think changing the behavior could result in a footgun where people use it thinking that they are being shown private keys when they actually are not.
I think there has been a misunderstanding. **There are private keys to show.** This PR doesn't change the behaviour of `li
...
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2878821724)
> Concept NACK
>
> The purpose of the `private` argument is to show the private keys. If there are no private keys to show, I think it is a reasonable error to say that, rather than just showing the public descriptors. I think changing the behavior could result in a footgun where people use it thinking that they are being shown private keys when they actually are not.
I think there has been a misunderstanding. **There are private keys to show.** This PR doesn't change the behaviour of `li
...
💬 Eunovo commented on pull request "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2878828270)
> I think that's fine to show a result, but I don't think we should show any result if there are no private keys at all.
No result is shown if there are no private keys
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2878828270)
> I think that's fine to show a result, but I don't think we should show any result if there are no private keys at all.
No result is shown if there are no private keys
💬 Eunovo commented on pull request "Fix listdescriptors true fails with 'Can't get descriptor string' in non-watch-only descriptor wallet":
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2878844076)
> It's unclear why someone should receive public keys with `private=true`. Why not just use the default command, `bitcoin-cli listdescriptors`, which already shows public keys? I assume the user is certain about the nature of the descriptors they have in their wallet. Maybe if I do not fully understand this PR.
It is possible to have descriptors with missing private keys in a non-watchonly wallet. `importdescriptors` only rejects descriptors without any private keys. Trying `listdescriptors p
...
(https://github.com/bitcoin/bitcoin/pull/32471#issuecomment-2878844076)
> It's unclear why someone should receive public keys with `private=true`. Why not just use the default command, `bitcoin-cli listdescriptors`, which already shows public keys? I assume the user is certain about the nature of the descriptors they have in their wallet. Maybe if I do not fully understand this PR.
It is possible to have descriptors with missing private keys in a non-watchonly wallet. `importdescriptors` only rejects descriptors without any private keys. Trying `listdescriptors p
...
💬 Sjors commented on pull request "Remove Taproot activation height":
(https://github.com/bitcoin/bitcoin/pull/26201#issuecomment-2878943691)
Trivial rebase after #32386.
(https://github.com/bitcoin/bitcoin/pull/26201#issuecomment-2878943691)
Trivial rebase after #32386.
💬 Sjors commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2088206037)
> a clear explanation of whether the check is redundant or necessary
The problem is that I don't know for sure. I _think_ it's redundant, but I don't want to mislead some future dev. They should do their own research.
> Even without this PR ... someone could dig into the code and eventually realize that it’s an incomplete check
The comment is there to prevent someone who _doesn't_ check from accidentally introducing a consensus bug.
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2088206037)
> a clear explanation of whether the check is redundant or necessary
The problem is that I don't know for sure. I _think_ it's redundant, but I don't want to mislead some future dev. They should do their own research.
> Even without this PR ... someone could dig into the code and eventually realize that it’s an incomplete check
The comment is there to prevent someone who _doesn't_ check from accidentally introducing a consensus bug.
💬 l0rinc commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2088212243)
I think we should add tests to avoid consensus bugs, not dead comments - or maybe both, but we definitely shouldn't rely on comments
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2088212243)
I think we should add tests to avoid consensus bugs, not dead comments - or maybe both, but we definitely shouldn't rely on comments
💬 Sjors commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2088223209)
Unfortunately this function barely has any test coverage. I'm adding some in #31981, but until then a comment is better than nothing.
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2088223209)
Unfortunately this function barely has any test coverage. I'm adding some in #31981, but until then a comment is better than nothing.
💬 polespinasa commented on pull request "rpc: generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2879033872)
> Perhaps we can make the second argument of `generateblock` optional, and if there is no set of txs provided then mine the mempool
That could be a smart approach. But I would always take the mempool or also put an optional argument to mine the mempool so it is not one or the other.
> I don't think we should be duplicating the effort implementing this on more than one RPC. Then every new mining test feature needs to be duplicated (or triplicated for `generatetodescriptor`/`generatetomany`)
...
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2879033872)
> Perhaps we can make the second argument of `generateblock` optional, and if there is no set of txs provided then mine the mempool
That could be a smart approach. But I would always take the mempool or also put an optional argument to mine the mempool so it is not one or the other.
> I don't think we should be duplicating the effort implementing this on more than one RPC. Then every new mining test feature needs to be duplicated (or triplicated for `generatetodescriptor`/`generatetomany`)
...
📝 maflcko opened a pull request: "refactor: Remove UB in prevector reverse iterators"
(https://github.com/bitcoin/bitcoin/pull/32490)
`rend()` creates a pointer with offset `-1`. This is UB, according to the C++ standard: https://eel.is/c++draft/expr.add#4:
When an expression J that has integral type is added to [...] an
expression P of pointer type, the result has the type of P.
... if P points to a (possibly-hypothetical) array element i of an
array object x with n elements [...] the expressions P + J and J + P
(where J has the value j) point to the (possibly-hypothetical) array
element i+j
...
(https://github.com/bitcoin/bitcoin/pull/32490)
`rend()` creates a pointer with offset `-1`. This is UB, according to the C++ standard: https://eel.is/c++draft/expr.add#4:
When an expression J that has integral type is added to [...] an
expression P of pointer type, the result has the type of P.
... if P points to a (possibly-hypothetical) array element i of an
array object x with n elements [...] the expressions P + J and J + P
(where J has the value j) point to the (possibly-hypothetical) array
element i+j
...