💬 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_
...
💬 delta1 commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2879319514)
@gmaxwell drahtbot has categorized your comment as nack instead of ack
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2879319514)
@gmaxwell drahtbot has categorized your comment as nack instead of ack
👍 TheCharlatan approved a pull request: "checkqueue: make the queue non-optional for CCheckQueueControl and drop legacy locking macro usage"
(https://github.com/bitcoin/bitcoin/pull/32467#pullrequestreview-2839318576)
ACK da5d55227e6aead8a9fbb85e967801cfcd4283ce
(https://github.com/bitcoin/bitcoin/pull/32467#pullrequestreview-2839318576)
ACK da5d55227e6aead8a9fbb85e967801cfcd4283ce
💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2088424348)
Works like a charm.
I also briefly tested `bitcoin.exe gui`, `node` and `rpc`.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2088424348)
Works like a charm.
I also briefly tested `bitcoin.exe gui`, `node` and `rpc`.
📝 fanquake opened a pull request: "build: document why we check for `std::system`"
(https://github.com/bitcoin/bitcoin/pull/32491)
It's probably debatable if we support targets like iOS, but for now, document why we are checking for this standard library feature.
Trying to use `std::system` for a `aarch64-darwin-ios` target results in:
```bash
test.cpp:7:10: error: 'system' is unavailable: not available on iOS
7 | std::system("some_command");
| ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/_stdlib.h:203:6: note: 'system' has been explicitly marked unavailable here
203 |
...
(https://github.com/bitcoin/bitcoin/pull/32491)
It's probably debatable if we support targets like iOS, but for now, document why we are checking for this standard library feature.
Trying to use `std::system` for a `aarch64-darwin-ios` target results in:
```bash
test.cpp:7:10: error: 'system' is unavailable: not available on iOS
7 | std::system("some_command");
| ^
/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/_stdlib.h:203:6: note: 'system' has been explicitly marked unavailable here
203 |
...
✅ fanquake closed an issue: "Mistake in `feature_pruning.py` functional test?"
(https://github.com/bitcoin/bitcoin/issues/32249)
(https://github.com/bitcoin/bitcoin/issues/32249)
💬 fanquake commented on issue "Mistake in `feature_pruning.py` functional test?":
(https://github.com/bitcoin/bitcoin/issues/32249#issuecomment-2879402851)
Closed via #32312.
(https://github.com/bitcoin/bitcoin/issues/32249#issuecomment-2879402851)
Closed via #32312.
💬 fanquake commented on pull request "test: Fix feature_pruning test after nTime typo fix":
(https://github.com/bitcoin/bitcoin/pull/32312#issuecomment-2879403090)
Backported to 29.x in #32292.
(https://github.com/bitcoin/bitcoin/pull/32312#issuecomment-2879403090)
Backported to 29.x in #32292.