💬 maflcko commented on pull request "Catch harmful arbitrary data. (missing code for #32359)":
(https://github.com/bitcoin/bitcoin/pull/32368#issuecomment-2850201910)
* This was opened with failing tests, with no further action for more than a week. (Fixing the tests, or at least running them to confirm they are passing at a minimum before opening a pull request should be trivial)
* It is a duplicate of a closed pull request, which was open in total for more than half a year, but never had a single code-review ack and passing tests/CI at the same time. Also, outstanding feedback from reviewers, such as https://github.com/bitcoin/bitcoin/pull/29520#discussion
...
(https://github.com/bitcoin/bitcoin/pull/32368#issuecomment-2850201910)
* This was opened with failing tests, with no further action for more than a week. (Fixing the tests, or at least running them to confirm they are passing at a minimum before opening a pull request should be trivial)
* It is a duplicate of a closed pull request, which was open in total for more than half a year, but never had a single code-review ack and passing tests/CI at the same time. Also, outstanding feedback from reviewers, such as https://github.com/bitcoin/bitcoin/pull/29520#discussion
...
💬 ajtowns commented on pull request "[WIP] rpc: add `clearmempool` command for regtest mode":
(https://github.com/bitcoin/bitcoin/pull/32418#issuecomment-2850249789)
Previously proposed in #13836
Wouldn't it be better to add an option to abandontransaction that tells it to first delete the transaction from the mempool? That could even be useful for mainnet, perhaps?
(https://github.com/bitcoin/bitcoin/pull/32418#issuecomment-2850249789)
Previously proposed in #13836
Wouldn't it be better to add an option to abandontransaction that tells it to first delete the transaction from the mempool? That could even be useful for mainnet, perhaps?
💬 stratospher commented on pull request "test: add test for malleated transaction with valid witness":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2073091815)
> Could use elif, but as the effect is the same, better to roll this into one if, i think:
nice, I've used this.
> All in all this defines not low_s as "high_s" instead of "any s". This doesn't cause problems as the test framework doesn't use low_s=False anywhere else. Still, it might be clearer to add a new flag?
oh right - just added a comment for now, since the low_s=False option isn't used elsewhere and can't think of a scenario where non-determinism from allowing "any s" (returnin
...
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2073091815)
> Could use elif, but as the effect is the same, better to roll this into one if, i think:
nice, I've used this.
> All in all this defines not low_s as "high_s" instead of "any s". This doesn't cause problems as the test framework doesn't use low_s=False anywhere else. Still, it might be clearer to add a new flag?
oh right - just added a comment for now, since the low_s=False option isn't used elsewhere and can't think of a scenario where non-determinism from allowing "any s" (returnin
...
💬 laanwj commented on pull request "test: add test for malleated transaction with valid witness":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2073106755)
Right it's fine, it's documented well enough now, if anyone needs that functionality in the future they can add it then.
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2073106755)
Right it's fine, it's documented well enough now, if anyone needs that functionality in the future they can add it then.
💬 purpleKarrot commented on pull request "build: replace header checks with `__has_include`":
(https://github.com/bitcoin/bitcoin/pull/32405#issuecomment-2850494903)
ACK e1f543823b300b28c9edaf5d1a3e1e9badde471b
Less code, less cmake cache variables, less error prone. However, I have concerns about the *if-header-exists-then-include-it* pattern, irrespective of the way the existence is checked (both `check_include_file_cxx` and `__has_include`). You don't include a header because it exists, but because of the symbols it declares. The logic should be: If the platform is `BAR`, then include `<foo.h>` to use function `foo` and fail when it it does not exist.
...
(https://github.com/bitcoin/bitcoin/pull/32405#issuecomment-2850494903)
ACK e1f543823b300b28c9edaf5d1a3e1e9badde471b
Less code, less cmake cache variables, less error prone. However, I have concerns about the *if-header-exists-then-include-it* pattern, irrespective of the way the existence is checked (both `check_include_file_cxx` and `__has_include`). You don't include a header because it exists, but because of the symbols it declares. The logic should be: If the platform is `BAR`, then include `<foo.h>` to use function `foo` and fail when it it does not exist.
...
🤔 TheCharlatan reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2814380926)
Approach ACK
I've been thinking about this approach over the approach @darosior suggested in #30983 again, and think this would indeed be cleaner. While providing multiprocess binaries over a known interface (e.g. just invoking the current binaries) could be beneficial in the initial roll out, I also like how this binary could potentially be the point where everything is packaged together if we ever do choose to go the full process + repository separation route. I think combining multiprocess
...
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2814380926)
Approach ACK
I've been thinking about this approach over the approach @darosior suggested in #30983 again, and think this would indeed be cleaner. While providing multiprocess binaries over a known interface (e.g. just invoking the current binaries) could be beneficial in the initial roll out, I also like how this binary could potentially be the point where everything is packaged together if we ever do choose to go the full process + repository separation route. I think combining multiprocess
...
💬 TheCharlatan commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2073194474)
I think it would be good to check that the file actually exists here. If in future releases more commands are added, and the user applies a command for it on a binary directory with the old version, the error is a bit terse. E.g. removing bitcoind will currently result in:
```
./bitcoin daemon --signet
Error: execvp failed to execute '/home/drgrid/bitcoin/build_dev_mode_clang/bin/bitcoind': No such file or directory
Try './bitcoin --help' for more information.
```
We already print "Unrecog
...
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2073194474)
I think it would be good to check that the file actually exists here. If in future releases more commands are added, and the user applies a command for it on a binary directory with the old version, the error is a bit terse. E.g. removing bitcoind will currently result in:
```
./bitcoin daemon --signet
Error: execvp failed to execute '/home/drgrid/bitcoin/build_dev_mode_clang/bin/bitcoind': No such file or directory
Try './bitcoin --help' for more information.
```
We already print "Unrecog
...
💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2073215058)
Should it be checking all the manifests or is just checking bitcoind.exe's enough?
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2073215058)
Should it be checking all the manifests or is just checking bitcoind.exe's enough?
💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2850592767)
We might also want to add a check for correct manifests to the symbols / security checks in the guix build. No idea if LIEF makes this easy or not.
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2850592767)
We might also want to add a check for correct manifests to the symbols / security checks in the guix build. No idea if LIEF makes this easy or not.
💬 purpleKarrot commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2850642681)
The best approach to respect user-provided flags is to treat **all** `CMAKE_...` variables as **read-only**.
The only place where those variables may be set, is *outside the project* (ie: env vars, toolchain files, initial cache, cli arguments).
Consider you have a function in C. Inside that function, you have to perform a division by one of the arguments. Since you cannot divide by zero, you treat the value `0` *as if* the user provided the value `2` instead. Would you consider that a valid
...
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2850642681)
The best approach to respect user-provided flags is to treat **all** `CMAKE_...` variables as **read-only**.
The only place where those variables may be set, is *outside the project* (ie: env vars, toolchain files, initial cache, cli arguments).
Consider you have a function in C. Inside that function, you have to perform a division by one of the arguments. Since you cannot divide by zero, you treat the value `0` *as if* the user provided the value `2` instead. Would you consider that a valid
...
💬 fanquake commented on pull request "build: replace header checks with `__has_include`":
(https://github.com/bitcoin/bitcoin/pull/32405#issuecomment-2850645193)
> The logic should be: If the platform is BAR, then include <foo.h>
Not sure I agree here; as this would give us less-generic code, and the need to maintain platform mappings.
(https://github.com/bitcoin/bitcoin/pull/32405#issuecomment-2850645193)
> The logic should be: If the platform is BAR, then include <foo.h>
Not sure I agree here; as this would give us less-generic code, and the need to maintain platform mappings.
💬 Sjors commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2850660569)
To clarify, is this PR supposed to write to disk _during_ IBD? My own testing with a very large -dbcache doesn't show that, since the chainstate directory remains 16 kb for at least a few hours.
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2850660569)
To clarify, is this PR supposed to write to disk _during_ IBD? My own testing with a very large -dbcache doesn't show that, since the chainstate directory remains 16 kb for at least a few hours.
🤔 TheCharlatan reviewed a pull request: "build: replace header checks with `__has_include`"
(https://github.com/bitcoin/bitcoin/pull/32405#pullrequestreview-2814491794)
Approach ACK e1f543823b300b28c9edaf5d1a3e1e9badde471b , pending guix build.
> You don't include a header because it exists, but because of the symbols it declares. The logic should be: If the platform is BAR, then include <foo.h> to use function foo and fail when it it does not exist. If not on platform BAR, then don't include <foo.h>.
There is already some additional gating in the first instance (randomenv.cpp), which actually checks for the relevant gadgets (HAVE_SYSCTL). A similar gatin
...
(https://github.com/bitcoin/bitcoin/pull/32405#pullrequestreview-2814491794)
Approach ACK e1f543823b300b28c9edaf5d1a3e1e9badde471b , pending guix build.
> You don't include a header because it exists, but because of the symbols it declares. The logic should be: If the platform is BAR, then include <foo.h> to use function foo and fail when it it does not exist. If not on platform BAR, then don't include <foo.h>.
There is already some additional gating in the first instance (randomenv.cpp), which actually checks for the relevant gadgets (HAVE_SYSCTL). A similar gatin
...
💬 Eunovo commented on pull request "descriptor: handle listdescriptors(private=true) for descriptors having partial keys":
(https://github.com/bitcoin/bitcoin/pull/32186#issuecomment-2850676440)
@rkrux still working on this?
(https://github.com/bitcoin/bitcoin/pull/32186#issuecomment-2850676440)
@rkrux still working on this?
👍 laanwj approved a pull request: "subprocess: Backport upstream changes"
(https://github.com/bitcoin/bitcoin/pull/32358#pullrequestreview-2814499302)
Code review re-ACK cd95c9d6a7ec08cca0f9c98328c759be586720f8
Reviewed:
```
git range-diff d62c2d82e14d27307d8790fd9d921b740b784668..7e80030a952a06101d5755032ebb1ff15823815e 5b8046a6e893b7fad5a93631e6d1e70db31878af..cd95c9d6a7ec08cca0f9c98328c759be586720f8
```
- 2fd3f2fec67a3bb62378c286fbf9667e6fb3cc3b added
- bb9ffea53fb021580f069c431aee02f547039831 added
- df7a52241f7dd7e995e5971abf67c293fc667144 → 174bd43f2e46a0ccc6f5ad486bb587c72c1241c3 was missing a `subprocess_close`
(https://github.com/bitcoin/bitcoin/pull/32358#pullrequestreview-2814499302)
Code review re-ACK cd95c9d6a7ec08cca0f9c98328c759be586720f8
Reviewed:
```
git range-diff d62c2d82e14d27307d8790fd9d921b740b784668..7e80030a952a06101d5755032ebb1ff15823815e 5b8046a6e893b7fad5a93631e6d1e70db31878af..cd95c9d6a7ec08cca0f9c98328c759be586720f8
```
- 2fd3f2fec67a3bb62378c286fbf9667e6fb3cc3b added
- bb9ffea53fb021580f069c431aee02f547039831 added
- df7a52241f7dd7e995e5971abf67c293fc667144 → 174bd43f2e46a0ccc6f5ad486bb587c72c1241c3 was missing a `subprocess_close`
🚀 hebasto merged a pull request: "subprocess: Backport upstream changes"
(https://github.com/bitcoin/bitcoin/pull/32358)
(https://github.com/bitcoin/bitcoin/pull/32358)
👋 hebasto's pull request is ready for review: "Reintroduce external signer support for Windows"
(https://github.com/bitcoin/bitcoin/pull/29868)
(https://github.com/bitcoin/bitcoin/pull/29868)
💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2850723656)
Rebased and undrafted.
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2850723656)
Rebased and undrafted.
📝 rkrux opened a pull request: "psbt: clarify PSBT, PSBTInput, PSBTOutput unserialization flows"
(https://github.com/bitcoin/bitcoin/pull/32419)
The unserialization flows of the PSBT types work based on few underlying assumptions of functions from `serialize.h` & `stream.h` that takes some to understand when read the first time.
Add few comments that highlight these assumptions hopefully making it easier to grasp. Also, mention key/value format types as per BIP 174.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-rel
...
(https://github.com/bitcoin/bitcoin/pull/32419)
The unserialization flows of the PSBT types work based on few underlying assumptions of functions from `serialize.h` & `stream.h` that takes some to understand when read the first time.
Add few comments that highlight these assumptions hopefully making it easier to grasp. Also, mention key/value format types as per BIP 174.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-rel
...
✅ rkrux closed a pull request: "descriptor: handle listdescriptors(private=true) for descriptors having partial keys"
(https://github.com/bitcoin/bitcoin/pull/32186)
(https://github.com/bitcoin/bitcoin/pull/32186)