💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2072702883)
Whenever I tried going back to pre-cmake times, I couldn't get it to compile and run again (on Mac or on Linux). Not the first time I try to do a git bisect, but it always turns bad really quickly.
I tried pushing a PR to my fork with the [original PR](https://github.com/bitcoin/bitcoin/pull/28865) that introduced the suppressions + removing `DynamicMemoryUsage`, `SpendCoin` and `Uncache` suppressionss from ubsan.
The build is [failing with similar problems](https://github.com/l0rinc/bitco
...
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2072702883)
Whenever I tried going back to pre-cmake times, I couldn't get it to compile and run again (on Mac or on Linux). Not the first time I try to do a git bisect, but it always turns bad really quickly.
I tried pushing a PR to my fork with the [original PR](https://github.com/bitcoin/bitcoin/pull/28865) that introduced the suppressions + removing `DynamicMemoryUsage`, `SpendCoin` and `Uncache` suppressionss from ubsan.
The build is [failing with similar problems](https://github.com/l0rinc/bitco
...
💬 naiyoma commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r2072704239)
nit: IMO, this could be a bit more descriptive
```diff
// Can't merge < 2 items
if (txs.size() < 2) {
- throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Missing transactions");
+ throw JSONRPCError(
+ RPC_INVALID_PARAMETER,
+ txs.size() == 0
+ ? "Missing transactions. At least two transactions required."
+ : "Insufficient transactions. At least two transactions required."
+ );
}
```
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r2072704239)
nit: IMO, this could be a bit more descriptive
```diff
// Can't merge < 2 items
if (txs.size() < 2) {
- throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Missing transactions");
+ throw JSONRPCError(
+ RPC_INVALID_PARAMETER,
+ txs.size() == 0
+ ? "Missing transactions. At least two transactions required."
+ : "Insufficient transactions. At least two transactions required."
+ );
}
```
✅ furszy closed a pull request: "[WIP] wallet: standardize change output detection process"
(https://github.com/bitcoin/bitcoin/pull/25979)
(https://github.com/bitcoin/bitcoin/pull/25979)
💬 furszy commented on pull request "[WIP] wallet: standardize change output detection process":
(https://github.com/bitcoin/bitcoin/pull/25979#issuecomment-2849481894)
> The dependency was closed more than a year ago: [#27601 (comment)](https://github.com/bitcoin/bitcoin/pull/27601#issuecomment-1989555740)
Time flies. Closing for now as it is not in my priorities.
(https://github.com/bitcoin/bitcoin/pull/25979#issuecomment-2849481894)
> The dependency was closed more than a year ago: [#27601 (comment)](https://github.com/bitcoin/bitcoin/pull/27601#issuecomment-1989555740)
Time flies. Closing for now as it is not in my priorities.
📝 cmdruid opened a pull request: "[WIP] rpc: add `clearmempool` command for regtest mode"
(https://github.com/bitcoin/bitcoin/pull/32418)
## Description
This commit adds a new RPC command called `clearmempool`. It is designed to remove all tranasctions from the mempool (including those from invalidated blocks), and return their TXIDs.
The purpose of this command is to clear the mempool, then use the returned TXIDs to perform additional cleanup.
When used in combination with `invalidateblock`, it allows me to roll-back the chain to a particular height, clear the mempool, then properly clean up wallets with `abandontransact
...
(https://github.com/bitcoin/bitcoin/pull/32418)
## Description
This commit adds a new RPC command called `clearmempool`. It is designed to remove all tranasctions from the mempool (including those from invalidated blocks), and return their TXIDs.
The purpose of this command is to clear the mempool, then use the returned TXIDs to perform additional cleanup.
When used in combination with `invalidateblock`, it allows me to roll-back the chain to a particular height, clear the mempool, then properly clean up wallets with `abandontransact
...
💬 andrewtoth commented on pull request "[WIP] rpc: add `clearmempool` command for regtest mode":
(https://github.com/bitcoin/bitcoin/pull/32418#issuecomment-2849691671)
> I need the ability to cleanly rollback the chain and wallets during testing
What about invalidating the block, recording all mempool txs via `getrawmempool`, stopping the node, deleting `mempool.dat` and restarting?
(https://github.com/bitcoin/bitcoin/pull/32418#issuecomment-2849691671)
> I need the ability to cleanly rollback the chain and wallets during testing
What about invalidating the block, recording all mempool txs via `getrawmempool`, stopping the node, deleting `mempool.dat` and restarting?
🤔 shahsb reviewed a pull request: "validation: periodically flush dbcache during reindex-chainstate"
(https://github.com/bitcoin/bitcoin/pull/32414#pullrequestreview-2813733894)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32414#pullrequestreview-2813733894)
Concept ACK
💬 starius commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2849942977)
I think I discovered a bug in existing test `test/functional/feature_signet.py`.
I found that it generates a block. I added the code to make sure that the block is distributed between the two nodes (node 0 and node 1) using the same signet challenge:
```diff
diff --git a/test/functional/feature_signet.py b/test/functional/feature_signet.py
index 02e37f0fdd..86ab3202b8 100755
--- a/test/functional/feature_signet.py
+++ b/test/functional/feature_signet.py
@@ -87,7 +87,10 @@ class SignetBasi
...
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2849942977)
I think I discovered a bug in existing test `test/functional/feature_signet.py`.
I found that it generates a block. I added the code to make sure that the block is distributed between the two nodes (node 0 and node 1) using the same signet challenge:
```diff
diff --git a/test/functional/feature_signet.py b/test/functional/feature_signet.py
index 02e37f0fdd..86ab3202b8 100755
--- a/test/functional/feature_signet.py
+++ b/test/functional/feature_signet.py
@@ -87,7 +87,10 @@ class SignetBasi
...
🚀 hebasto merged a pull request: "doc: Explain that .gitignore is not for IDE-specific excludes"
(https://github.com/bitcoin/bitcoin/pull/32417)
(https://github.com/bitcoin/bitcoin/pull/32417)
✅ maflcko closed a pull request: "Catch harmful arbitrary data. (missing code for #32359)"
(https://github.com/bitcoin/bitcoin/pull/32368)
(https://github.com/bitcoin/bitcoin/pull/32368)
💬 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
...