💬 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
...
💬 Sjors commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#issuecomment-2879192945)
I still found the comment a bit confusing: https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2086713176
Maybe it's better to also move the `GetTip()` implementation to the miner, so we call it from the WaitTipChanged implementation. See https://github.com/Sjors/bitcoin/commits/2025/05/interface/
If you then squash the last commit into 51344920ebe1772d8dd3bece789a6510d774ab59, the result is a smaller change to the `waitTipChanged` body, see 2dae1cb16e3e32aa477713b88adcfbb6251cc779.
...
(https://github.com/bitcoin/bitcoin/pull/32378#issuecomment-2879192945)
I still found the comment a bit confusing: https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2086713176
Maybe it's better to also move the `GetTip()` implementation to the miner, so we call it from the WaitTipChanged implementation. See https://github.com/Sjors/bitcoin/commits/2025/05/interface/
If you then squash the last commit into 51344920ebe1772d8dd3bece789a6510d774ab59, the result is a smaller change to the `waitTipChanged` body, see 2dae1cb16e3e32aa477713b88adcfbb6251cc779.
...
👍 maflcko approved a pull request: "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`"
(https://github.com/bitcoin/bitcoin/pull/32296#pullrequestreview-2839190115)
nice, lgtm
(https://github.com/bitcoin/bitcoin/pull/32296#pullrequestreview-2839190115)
nice, lgtm
💬 maflcko commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2088333853)
style nit: Could adjust the indent manually, while touching this, so that the `ser_writedata*` are aligned? Also, personally I prefer to use `uint8_t(a)` over the verbose static cast for integral types. See https://github.com/bitcoin/bitcoin/blob/f9d8910539a2f7c4542457e08cd2bdd2dcb9cd08/doc/developer-notes.md#L111
But just style nits, up to you.
(https://github.com/bitcoin/bitcoin/pull/32296#discussion_r2088333853)
style nit: Could adjust the indent manually, while touching this, so that the `ser_writedata*` are aligned? Also, personally I prefer to use `uint8_t(a)` over the verbose static cast for integral types. See https://github.com/bitcoin/bitcoin/blob/f9d8910539a2f7c4542457e08cd2bdd2dcb9cd08/doc/developer-notes.md#L111
But just style nits, up to you.
💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2088367575)
Good point, otherwise it leaves `bitcoin.exe` behind because it's not inside the `daemon` subdirectory.
I'll add this to my Windows test:
```
diff --git a/share/setup.nsi.in b/share/setup.nsi.in
index 270468ad7b..2160cc05a1 100644
--- a/share/setup.nsi.in
+++ b/share/setup.nsi.in
@@ -130,6 +130,7 @@ done${UNSECTION_ID}:
# Uninstaller sections
Section /o -un.Main UNSEC0000
+ Delete /REBOOTOK $INSTDIR\@BITCOIN_WRAPPER_NAME@@EXEEXT@
Delete /REBOOTOK $INSTDIR\@BITCOIN_GUI_
...
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2088367575)
Good point, otherwise it leaves `bitcoin.exe` behind because it's not inside the `daemon` subdirectory.
I'll add this to my Windows test:
```
diff --git a/share/setup.nsi.in b/share/setup.nsi.in
index 270468ad7b..2160cc05a1 100644
--- a/share/setup.nsi.in
+++ b/share/setup.nsi.in
@@ -130,6 +130,7 @@ done${UNSECTION_ID}:
# Uninstaller sections
Section /o -un.Main UNSEC0000
+ Delete /REBOOTOK $INSTDIR\@BITCOIN_WRAPPER_NAME@@EXEEXT@
Delete /REBOOTOK $INSTDIR\@BITCOIN_GUI_
...