📝 fanquake opened a pull request: "contrib: simplify `test-security-check`"
(https://github.com/bitcoin/bitcoin/pull/30423)
The current `test-security-check` script is hard to understand, and change (i.e https://github.com/bitcoin/bitcoin/pull/29987/files#diff-52aa0cda44721f089e53b128cb1232a876006ef257b211655456b17dfb2ec712); tests are also not done in isolation (when-possible). Fix that, and add missing checks. Simplifies future toolchain/security/hardening changes.
(https://github.com/bitcoin/bitcoin/pull/30423)
The current `test-security-check` script is hard to understand, and change (i.e https://github.com/bitcoin/bitcoin/pull/29987/files#diff-52aa0cda44721f089e53b128cb1232a876006ef257b211655456b17dfb2ec712); tests are also not done in isolation (when-possible). Fix that, and add missing checks. Simplifies future toolchain/security/hardening changes.
👍 hebasto approved a pull request: "util: Catch translation string errors at compile time"
(https://github.com/bitcoin/bitcoin/pull/30383#pullrequestreview-2169704590)
ACK fa601ab9f7142cdb18c18c1128fc893cdffb3463.
nit: The error messages for the cases `_(0);` and `_(NULL);` can be confusing a bit. For instance, clang-18 reports "conversion from 'long' to 'ConstevalStringLiteral' is ambiguous" for the latter.
(https://github.com/bitcoin/bitcoin/pull/30383#pullrequestreview-2169704590)
ACK fa601ab9f7142cdb18c18c1128fc893cdffb3463.
nit: The error messages for the cases `_(0);` and `_(NULL);` can be confusing a bit. For instance, clang-18 reports "conversion from 'long' to 'ConstevalStringLiteral' is ambiguous" for the latter.
💬 brunoerg commented on pull request "fuzz: Version handshake":
(https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2220979817)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30413#issuecomment-2220979817)
Concept ACK
💬 hebasto commented on pull request "contrib: simplify `test-security-check`":
(https://github.com/bitcoin/bitcoin/pull/30423#issuecomment-2220993948)
> tests are also not done in isolation (when-possible).
Mind elaborating this statement please? I mean, `contrib/devtools/test-security-check.py` is supposed to run in the Guix environment. So what kind of "isolation" do you mean?
(https://github.com/bitcoin/bitcoin/pull/30423#issuecomment-2220993948)
> tests are also not done in isolation (when-possible).
Mind elaborating this statement please? I mean, `contrib/devtools/test-security-check.py` is supposed to run in the Guix environment. So what kind of "isolation" do you mean?
💬 cobratbq commented on pull request "doc: external-signer, example 'createwallet' RPC call requires eight argument":
(https://github.com/bitcoin/bitcoin/pull/30354#issuecomment-2221003676)
> Highly likely it was always broken. I'd be surprised if a PR that added a positional argument in the middle would make it through code review.
Okay, that's clear. Well we can fix it now, and add the named arguments.
> > The docs on descriptors (`descriptors.md`) also show examples, such as `pk(xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8)`, and I have no idea whether this is supposed to be supported or not.
>
> That su
...
(https://github.com/bitcoin/bitcoin/pull/30354#issuecomment-2221003676)
> Highly likely it was always broken. I'd be surprised if a PR that added a positional argument in the middle would make it through code review.
Okay, that's clear. Well we can fix it now, and add the named arguments.
> > The docs on descriptors (`descriptors.md`) also show examples, such as `pk(xpub661MyMwAqRbcFtXgS5sYJABqqG9YLmC4Q1Rdap9gSE8NqtwybGhePY2gZ29ESFjqJoCu1Rupje8YtGqsefD265TMg7usUDFdp6W1EGMcet8)`, and I have no idea whether this is supposed to be supported or not.
>
> That su
...
💬 maflcko commented on pull request "util: Catch translation string errors at compile time":
(https://github.com/bitcoin/bitcoin/pull/30383#issuecomment-2221012027)
> nit: The error messages for the cases `_(0);` and `_(NULL);` can be confusing a bit. For instance, clang-18 reports "conversion from 'long' to 'ConstevalStringLiteral' is ambiguous" for the latter.
Unrelated to this codebase, but I'd say this should be fixed upstream by doing `#define NULL nullptr` (under C++11 or later) instead of using an integer literal.
(https://github.com/bitcoin/bitcoin/pull/30383#issuecomment-2221012027)
> nit: The error messages for the cases `_(0);` and `_(NULL);` can be confusing a bit. For instance, clang-18 reports "conversion from 'long' to 'ConstevalStringLiteral' is ambiguous" for the latter.
Unrelated to this codebase, but I'd say this should be fixed upstream by doing `#define NULL nullptr` (under C++11 or later) instead of using an integer literal.
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1672669253)
> variation of it should actually probably go right here
Maybe, but I would still be surprised, regardless of the context, if `not X` leads to `X`. Could the parameter explain it better than "erase", so that the reviewer isn't surprised that it means "keep"?
> invalidate your ACK
I expect that I will have to review and ack your changes many more times before it's merged, don't worry about it!
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1672669253)
> variation of it should actually probably go right here
Maybe, but I would still be surprised, regardless of the context, if `not X` leads to `X`. Could the parameter explain it better than "erase", so that the reviewer isn't surprised that it means "keep"?
> invalidate your ACK
I expect that I will have to review and ack your changes many more times before it's merged, don't worry about it!
👍 itornaza approved a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2169749598)
tested ACK bdc2a656c2d2a61d226fde1d1fd4e79664106e18
Built from source and run all unit, functional and extended tests and all of them pass. Looked through the code changes and they look great to me. I particularly like the default definition of the move constructors in `KeyPair` since there is a pointer member `m_keydata` in that class, plus the fact that the generation of the copy constructors that are not needed anywhere is suppressed due to the use of the default keyword which I consider a
...
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2169749598)
tested ACK bdc2a656c2d2a61d226fde1d1fd4e79664106e18
Built from source and run all unit, functional and extended tests and all of them pass. Looked through the code changes and they look great to me. I particularly like the default definition of the move constructors in `KeyPair` since there is a pointer member `m_keydata` in that class, plus the fact that the generation of the copy constructors that are not needed anywhere is suppressed due to the use of the default keyword which I consider a
...
💬 itornaza commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1672656999)
The same header is already and very thoughtfully provided for the following function a few lines above
`KeyPair ComputeKeyPair(const uint256* merkle_root) const;`
Maybe here a shorter header would be more appropriate to avoid repeating ourselves and make maintenance easier to handle (at the comment level), like so
```
/** KeyPair
*
* Wrapper class for the `secp256k1_keypair` type */
*/ ```
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1672656999)
The same header is already and very thoughtfully provided for the following function a few lines above
`KeyPair ComputeKeyPair(const uint256* merkle_root) const;`
Maybe here a shorter header would be more appropriate to avoid repeating ourselves and make maintenance easier to handle (at the comment level), like so
```
/** KeyPair
*
* Wrapper class for the `secp256k1_keypair` type */
*/ ```
🤔 ismaelsadeeq reviewed a pull request: "test: fix inconsistency in fundrawtransaction weight limits test"
(https://github.com/bitcoin/bitcoin/pull/30353#pullrequestreview-2169806132)
Concept ACK
I could not recreate [this failure](https://cirrus-ci.com/task/5319226913718272?logs=ci#L1923) locally. Looking closely at the code, 4f571fb73526b55a83af3c04fbc662ccdd1307ae might not solve the issue.
For us to load coins from `AutomaticCoinSelection` and reach the failure in the C.I.
`selection_target` in `SelectCoins` must be greater than 0.
But when `selection_target` is > 0 and we prevent adding input with `m_allow_other_inputs=False`, just as it's done in 4f571
...
(https://github.com/bitcoin/bitcoin/pull/30353#pullrequestreview-2169806132)
Concept ACK
I could not recreate [this failure](https://cirrus-ci.com/task/5319226913718272?logs=ci#L1923) locally. Looking closely at the code, 4f571fb73526b55a83af3c04fbc662ccdd1307ae might not solve the issue.
For us to load coins from `AutomaticCoinSelection` and reach the failure in the C.I.
`selection_target` in `SelectCoins` must be greater than 0.
But when `selection_target` is > 0 and we prevent adding input with `m_allow_other_inputs=False`, just as it's done in 4f571
...
⚠️ maflcko opened an issue: "intermittent issue in feature_reindex.py: Reindex fails to continue after restart; `initload` thread hangs forever"
(https://github.com/bitcoin/bitcoin/issues/30424)
The goal of `continue_reindex_after_shutdown` is to start with `-reindex`, then stop as soon as possible and restart the node to pick up the reindex again, then stop the node as soon as possible again.
However, sometimes the second shutdown stalls forever (without progress). So it seems like there are several bugs:
* The shutdown stalls forever (it should interrupt and shut down in a reasonable time)
* There is no reindex progess (it should pick up the previous work and try to make proges
...
(https://github.com/bitcoin/bitcoin/issues/30424)
The goal of `continue_reindex_after_shutdown` is to start with `-reindex`, then stop as soon as possible and restart the node to pick up the reindex again, then stop the node as soon as possible again.
However, sometimes the second shutdown stalls forever (without progress). So it seems like there are several bugs:
* The shutdown stalls forever (it should interrupt and shut down in a reasonable time)
* There is no reindex progess (it should pick up the previous work and try to make proges
...
🚀 ryanofsky merged a pull request: "Fix issues with CI on forks"
(https://github.com/bitcoin/bitcoin/pull/29274)
(https://github.com/bitcoin/bitcoin/pull/29274)
💬 ryanofsky commented on pull request "kernel, refactor: return error status on all fatal errors":
(https://github.com/bitcoin/bitcoin/pull/29700#issuecomment-2221105889)
Rebased 692fc11f0aae3c8013f6f01d133139ce8eba7135 -> ad1e73590d102edf3738986c1382b5e36a0ed727 ([`pr/fatalresult.17`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.17) -> [`pr/fatalresult.18`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.18), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/fatalresult.17-rebase..pr/fatalresult.18)) to fix conflicts
(https://github.com/bitcoin/bitcoin/pull/29700#issuecomment-2221105889)
Rebased 692fc11f0aae3c8013f6f01d133139ce8eba7135 -> ad1e73590d102edf3738986c1382b5e36a0ed727 ([`pr/fatalresult.17`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.17) -> [`pr/fatalresult.18`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.18), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/fatalresult.17-rebase..pr/fatalresult.18)) to fix conflicts
💬 hebasto commented on pull request "Bugfix: Correct first-run free space checks":
(https://github.com/bitcoin/bitcoin/pull/29678#issuecomment-2221122443)
Indeed, there is inconsistency in GB/GiB unit usage.
For instance, the value of the `m_assumed_blockchain_size` variable in GiB is used with the "GB" units in the user-faced messages here: https://github.com/bitcoin/bitcoin/blob/45f757c72672fc351bf669e731744f2f5e269233/src/init.cpp#L1707 and here: https://github.com/bitcoin/bitcoin/blob/45f757c72672fc351bf669e731744f2f5e269233/src/qt/forms/intro.ui#L206
Concept ACK.
---
It would be nice to mention the second commit changes in the PR
...
(https://github.com/bitcoin/bitcoin/pull/29678#issuecomment-2221122443)
Indeed, there is inconsistency in GB/GiB unit usage.
For instance, the value of the `m_assumed_blockchain_size` variable in GiB is used with the "GB" units in the user-faced messages here: https://github.com/bitcoin/bitcoin/blob/45f757c72672fc351bf669e731744f2f5e269233/src/init.cpp#L1707 and here: https://github.com/bitcoin/bitcoin/blob/45f757c72672fc351bf669e731744f2f5e269233/src/qt/forms/intro.ui#L206
Concept ACK.
---
It would be nice to mention the second commit changes in the PR
...
💬 pinheadmz commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#discussion_r1672758736)
oh whoops, thanks. Should I move it now?
(https://github.com/bitcoin/bitcoin/pull/27064#discussion_r1672758736)
oh whoops, thanks. Should I move it now?
🤔 mzumsande reviewed a pull request: "net: fix race condition in self-connect detection"
(https://github.com/bitcoin/bitcoin/pull/30394#pullrequestreview-2169946051)
ACK 16bd283b3ad05daa41259a062aee0fc05b463fa6
> Our own test framework was behaving that way before #29353, so maybe the functional tests would have caught it? I thought about reverting [c340503](https://github.com/bitcoin/bitcoin/commit/c340503b67585b631909584f1acaa327f5af25d3) locally to find out, but didn't have the time yet.
FYI, I tested reverting c340503b67585b631909584f1acaa327f5af25d3 on top of this branch now and without the latest fix (2) to ProcessMessages, other messages are act
...
(https://github.com/bitcoin/bitcoin/pull/30394#pullrequestreview-2169946051)
ACK 16bd283b3ad05daa41259a062aee0fc05b463fa6
> Our own test framework was behaving that way before #29353, so maybe the functional tests would have caught it? I thought about reverting [c340503](https://github.com/bitcoin/bitcoin/commit/c340503b67585b631909584f1acaa327f5af25d3) locally to find out, but didn't have the time yet.
FYI, I tested reverting c340503b67585b631909584f1acaa327f5af25d3 on top of this branch now and without the latest fix (2) to ProcessMessages, other messages are act
...
🤔 paplorinc requested changes to a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2169947102)
Not yet sure about adding stuff that "will hopefully used this way one day".
Refactoring is welcome, as long as it's easy to review, which is not yet the case here, could you split the change into trivial, focused commits?
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2169947102)
Not yet sure about adding stuff that "will hopefully used this way one day".
Refactoring is welcome, as long as it's easy to review, which is not yet the case here, could you split the change into trivial, focused commits?
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1672775882)
Moving and modifying the code in the same commit (while many of the changes serve different purposes, mixing low and high-risk changes) makes the review a lot harder since the diff isn't really useful – having separate red and green parts, not really a diff.
Additionally, the commit doesn't explain each change separately. For example:
* The exact reason for memory management changes (via `make_secure_unique`) is not explained, nor are the security implications or potential performance impact
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1672775882)
Moving and modifying the code in the same commit (while many of the changes serve different purposes, mixing low and high-risk changes) makes the review a lot harder since the diff isn't really useful – having separate red and green parts, not really a diff.
Additionally, the commit doesn't explain each change separately. For example:
* The exact reason for memory management changes (via `make_secure_unique`) is not explained, nor are the security implications or potential performance impact
...
👍 TheCharlatan approved a pull request: "test: [refactor] Pass TestOpts"
(https://github.com/bitcoin/bitcoin/pull/30407#pullrequestreview-2169975415)
Nice, ACK fa690c8e532672f7ab53be6f7a0bb3070858745e
(https://github.com/bitcoin/bitcoin/pull/30407#pullrequestreview-2169975415)
Nice, ACK fa690c8e532672f7ab53be6f7a0bb3070858745e
📝 TheCharlatan opened a pull request: "kernel: De-globalize static validation variables"
(https://github.com/bitcoin/bitcoin/pull/30425)
In future, users of the kernel library might run multiple chainstates in parallel, or create and destroy multiple chainstates over the lifetime of a process. Having static, mutable variables could lead to state inconsistencies in these scenarios.
(https://github.com/bitcoin/bitcoin/pull/30425)
In future, users of the kernel library might run multiple chainstates in parallel, or create and destroy multiple chainstates over the lifetime of a process. Having static, mutable variables could lead to state inconsistencies in these scenarios.
💬 achow101 commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + test":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2221235072)
ACK 5b7f70ba2661a194a3c476b81e03785feddb4e1c
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2221235072)
ACK 5b7f70ba2661a194a3c476b81e03785feddb4e1c