π¬ davidgumberg commented on issue "test: functional test failures under -DENABLE_WALLET=OFF":
(https://github.com/bitcoin/bitcoin/issues/32347#issuecomment-2845668503)
I think #31250 genuinely fixed this, https://github.com/bitcoin/bitcoin/pull/31250/commits/c847dee1488a294c9a9632a00ba1134b21e41947, changed the logic for whether or not tests are run in all the crash cases exhibited here:
```diff
diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
index c7f0cc5e43..ee79a27392 100755
--- a/test/functional/feature_rbf.py
+++ b/test/functional/feature_rbf.py
@@ -567,7 +565,7 @@ class ReplaceByFeeTest(BitcoinTestFramework):
assert
...
(https://github.com/bitcoin/bitcoin/issues/32347#issuecomment-2845668503)
I think #31250 genuinely fixed this, https://github.com/bitcoin/bitcoin/pull/31250/commits/c847dee1488a294c9a9632a00ba1134b21e41947, changed the logic for whether or not tests are run in all the crash cases exhibited here:
```diff
diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
index c7f0cc5e43..ee79a27392 100755
--- a/test/functional/feature_rbf.py
+++ b/test/functional/feature_rbf.py
@@ -567,7 +565,7 @@ class ReplaceByFeeTest(BitcoinTestFramework):
assert
...
π¬ monlovesmango commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2070755965)
Ok I see! I understand now. Sorry was running with `--log_level=all` and was misinterpreting these as success messages. Please disregard..
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2070755965)
Ok I see! I understand now. Sorry was running with `--log_level=all` and was misinterpreting these as success messages. Please disregard..
π€ 1440000bytes reviewed a pull request: "Extend signetchallenge to set target block spacing"
(https://github.com/bitcoin/bitcoin/pull/29365#pullrequestreview-2810654144)
utACK https://github.com/bitcoin/bitcoin/pull/29365/commits/41728b516ec28ba2b6bbffd0b5e4cec8120f61d1
Isn't the commit message too long? You could even have multiple commits in this pull request IMO.
(https://github.com/bitcoin/bitcoin/pull/29365#pullrequestreview-2810654144)
utACK https://github.com/bitcoin/bitcoin/pull/29365/commits/41728b516ec28ba2b6bbffd0b5e4cec8120f61d1
Isn't the commit message too long? You could even have multiple commits in this pull request IMO.
π¬ hebasto commented on pull request "subprocess: Backport upstream changes":
(https://github.com/bitcoin/bitcoin/pull/32358#issuecomment-2845818802)
> > During my quick (and possibly not thorough) tests, I was unable to reproduce the issues mentioned in those PRs on our codebase.
>
> Seems odd to not pull in all changes, given no real rationale (i.e the code being incorrect/broken) for excluding them. This also means that next time this header is updated, they may or may not get pulled in anyways, depending on the author of the change, given there's nothing documented to say they should be excluded.
https://github.com/arun11299/cpp-sub
...
(https://github.com/bitcoin/bitcoin/pull/32358#issuecomment-2845818802)
> > During my quick (and possibly not thorough) tests, I was unable to reproduce the issues mentioned in those PRs on our codebase.
>
> Seems odd to not pull in all changes, given no real rationale (i.e the code being incorrect/broken) for excluding them. This also means that next time this header is updated, they may or may not get pulled in anyways, depending on the author of the change, given there's nothing documented to say they should be excluded.
https://github.com/arun11299/cpp-sub
...
π¬ starius commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2845902580)
> utACK [41728b5](https://github.com/bitcoin/bitcoin/commit/41728b516ec28ba2b6bbffd0b5e4cec8120f61d1)
>
> Isn't the commit message too long? You could even have multiple commits in this pull request IMO.
It is possible to split the commit (and the commit message) into multiple parts:
- add function `ParseWrappedSignetChallenge` and its unit test (files `src/chainparams.{h,cpp} src/test/chainparams_tests.cpp src/test/CMakeLists.txt`)
- add function `ParseSignetParams` and its unit t
...
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2845902580)
> utACK [41728b5](https://github.com/bitcoin/bitcoin/commit/41728b516ec28ba2b6bbffd0b5e4cec8120f61d1)
>
> Isn't the commit message too long? You could even have multiple commits in this pull request IMO.
It is possible to split the commit (and the commit message) into multiple parts:
- add function `ParseWrappedSignetChallenge` and its unit test (files `src/chainparams.{h,cpp} src/test/chainparams_tests.cpp src/test/CMakeLists.txt`)
- add function `ParseSignetParams` and its unit t
...
π¬ murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2070888721)
Thanks for taking such a thorough look!
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2070888721)
Thanks for taking such a thorough look!
π¬ davidgumberg commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r2070888806)
In commit ef83e9569752b4273e6aed3124642dc057cab540 (shutdown: Tear down mempool after chainman)
Feel free to disregard as out of scope, but while addressing this, it might be nice to make reintroducing this dangling pointer impossible by making the various mempool pointers `shared_ptr`, e.g. https://github.com/davidgumberg/bitcoin/commit/75c839b8d77c207a0946294672b703d9f8eecbe1
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r2070888806)
In commit ef83e9569752b4273e6aed3124642dc057cab540 (shutdown: Tear down mempool after chainman)
Feel free to disregard as out of scope, but while addressing this, it might be nice to make reintroducing this dangling pointer impossible by making the various mempool pointers `shared_ptr`, e.g. https://github.com/davidgumberg/bitcoin/commit/75c839b8d77c207a0946294672b703d9f8eecbe1
π¬ oxqnd commented on issue "combinerawtransaction confusing with distinct transactions":
(https://github.com/bitcoin/bitcoin/issues/25980#issuecomment-2845930776)
Iβll take a look at #31298, but it might take me a little while to get to it. Hope thatβs okay!
(https://github.com/bitcoin/bitcoin/issues/25980#issuecomment-2845930776)
Iβll take a look at #31298, but it might take me a little while to get to it. Hope thatβs okay!
π mzumsande opened a pull request: "cli: add -usefile option"
(https://github.com/bitcoin/bitcoin/pull/32399)
Running large commands via `bitcoin-cli` (such as submitting a large tx or block) runs into the `MAX_ARG_STRLEN` limit, which is 128KB on many systems. (see e.g. https://trofi.github.io/posts/299-maximum-argument-count-on-linux-and-in-gcc.html for more info).
This PR suggests to make it possible to bypass this limit by allowing to put the command and all of its args into a file. The functional test framework is then adjusted to automatically use this option (using a temporary file) if `--usec
...
(https://github.com/bitcoin/bitcoin/pull/32399)
Running large commands via `bitcoin-cli` (such as submitting a large tx or block) runs into the `MAX_ARG_STRLEN` limit, which is 128KB on many systems. (see e.g. https://trofi.github.io/posts/299-maximum-argument-count-on-linux-and-in-gcc.html for more info).
This PR suggests to make it possible to bypass this limit by allowing to put the command and all of its args into a file. The functional test framework is then adjusted to automatically use this option (using a temporary file) if `--usec
...
π¬ oxqnd commented on issue "combinerawtransaction confusing with distinct transactions":
(https://github.com/bitcoin/bitcoin/issues/25980#issuecomment-2845932631)
> > Could I try this issue?
>
> Would you mind reviewing https://github.com/bitcoin/bitcoin/pull/31298 instead?
>
> This solution is still up to date, it just hasn't gotten the right reviewers.
Iβll take a look at #31298, but it might take me a little while to get to it. Hope thatβs okay!
(https://github.com/bitcoin/bitcoin/issues/25980#issuecomment-2845932631)
> > Could I try this issue?
>
> Would you mind reviewing https://github.com/bitcoin/bitcoin/pull/31298 instead?
>
> This solution is still up to date, it just hasn't gotten the right reviewers.
Iβll take a look at #31298, but it might take me a little while to get to it. Hope thatβs okay!
π¬ rebroad commented on pull request "Enhanced Traffic Graph Widget with Multi-timeframe Support and Data Persistence":
(https://github.com/bitcoin-core/gui/pull/866#issuecomment-2845967863)
I've removed the code that changed the totals on the node - the GUI just keeps track now without writing values to the node. Also, added some checks for corrupt save files so that it doesn't load bad data.
(https://github.com/bitcoin-core/gui/pull/866#issuecomment-2845967863)
I've removed the code that changed the totals on the node - the GUI just keeps track now without writing values to the node. Also, added some checks for corrupt save files so that it doesn't load bad data.
π¬ achow101 commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2846003391)
ACK 85368aafa0d1772626d5f615e272b1c1a580b42f
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2846003391)
ACK 85368aafa0d1772626d5f615e272b1c1a580b42f
π¬ monlovesmango commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2846047880)
ACK 85368aafa0d1772626d5f615e272b1c1a580b42f
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2846047880)
ACK 85368aafa0d1772626d5f615e272b1c1a580b42f
π davidgumberg opened a pull request: "random: Use modern Windows randomness functions"
(https://github.com/bitcoin/bitcoin/pull/32400)
This resolves #32391 and is a follow-up to #14089.
The old randomness API has been deprecated and will be removed at some point.[^1]
For reference on `BCryptGenRandom`, see: https://learn.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom.
[`STATUS_SUCCESS`](https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-erref/596a1078-e883-4972-9bbc-49e60bebca55) gets defined here since including `ntstatus.h` is [more trouble](https://github.com/bitcoin-core/secp25
...
(https://github.com/bitcoin/bitcoin/pull/32400)
This resolves #32391 and is a follow-up to #14089.
The old randomness API has been deprecated and will be removed at some point.[^1]
For reference on `BCryptGenRandom`, see: https://learn.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom.
[`STATUS_SUCCESS`](https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-erref/596a1078-e883-4972-9bbc-49e60bebca55) gets defined here since including `ntstatus.h` is [more trouble](https://github.com/bitcoin-core/secp25
...
π¬ davidgumberg commented on issue "Use of deprecated CryptAcquireContext in random.cpp":
(https://github.com/bitcoin/bitcoin/issues/32391#issuecomment-2846087303)
Opened #32400 to resolve.
(https://github.com/bitcoin/bitcoin/issues/32391#issuecomment-2846087303)
Opened #32400 to resolve.
π¬ davidgumberg commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2070981899)
Thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2070981899)
Thanks, fixed.
π¬ davidgumberg commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2070981955)
Thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/31774#discussion_r2070981955)
Thanks, fixed.
π¬ davidgumberg commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2846099273)
Rebase to address style feedback.
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2846099273)
Rebase to address style feedback.
π€ shahsb reviewed a pull request: "random: Use modern Windows randomness functions"
(https://github.com/bitcoin/bitcoin/pull/32400#pullrequestreview-2811079433)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32400#pullrequestreview-2811079433)
Concept ACK
π€ shahsb reviewed a pull request: "random: Use modern Windows randomness functions"
(https://github.com/bitcoin/bitcoin/pull/32400#pullrequestreview-2811079804)
LGTM..!!
ACK https://github.com/bitcoin/bitcoin/pull/32400/commits/9dcf2105a9fa60b600145e3fe032a296d47b28e3
(https://github.com/bitcoin/bitcoin/pull/32400#pullrequestreview-2811079804)
LGTM..!!
ACK https://github.com/bitcoin/bitcoin/pull/32400/commits/9dcf2105a9fa60b600145e3fe032a296d47b28e3