✅ pinheadmz closed an issue: "npm install json-payment-protocol"
(https://github.com/bitcoin/bitcoin/issues/30422)
(https://github.com/bitcoin/bitcoin/issues/30422)
:lock: fanquake locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/30422)
(https://github.com/bitcoin/bitcoin/issues/30422)
👍 hebasto approved a pull request: "contrib: fix check-deps.sh to check for weak symbols"
(https://github.com/bitcoin/bitcoin/pull/30415#pullrequestreview-2169500423)
ACK 5a8b9432cde11f6140855717af195d8b7e798d75, tested on Ubuntu 24.04/
(https://github.com/bitcoin/bitcoin/pull/30415#pullrequestreview-2169500423)
ACK 5a8b9432cde11f6140855717af195d8b7e798d75, tested on Ubuntu 24.04/
💬 hebasto commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#discussion_r1672501712)
nit: Maybe make a reference to a PR consistent with the [previous one](https://github.com/bitcoin/bitcoin/blob/9adebe145557ef410964dd48a02f3d239f488cd0/contrib/devtools/check-deps.sh#L55)?
(https://github.com/bitcoin/bitcoin/pull/30415#discussion_r1672501712)
nit: Maybe make a reference to a PR consistent with the [previous one](https://github.com/bitcoin/bitcoin/blob/9adebe145557ef410964dd48a02f3d239f488cd0/contrib/devtools/check-deps.sh#L55)?
💬 rishkwal commented on issue "utils: add support for `bitcoin-wallet` tool configuration file":
(https://github.com/bitcoin/bitcoin/issues/30421#issuecomment-2220813745)
I see `bitcoin-wallet` as a distinct entity from `bitcoind`, `bitcoin-cli`, and `bitcoin-qt`, which are tightly coupled. `bitcoin-cli` already manages wallets alongside the node, but `bitcoin-wallet` offers independent wallet management. Having a separate `wallet.conf` allows for independent wallet management without altering node settings. It provides the flexibility to manage additional wallets without affecting the main wallet configured via bitcoin-cli and simplifies configuration for users
...
(https://github.com/bitcoin/bitcoin/issues/30421#issuecomment-2220813745)
I see `bitcoin-wallet` as a distinct entity from `bitcoind`, `bitcoin-cli`, and `bitcoin-qt`, which are tightly coupled. `bitcoin-cli` already manages wallets alongside the node, but `bitcoin-wallet` offers independent wallet management. Having a separate `wallet.conf` allows for independent wallet management without altering node settings. It provides the flexibility to manage additional wallets without affecting the main wallet configured via bitcoin-cli and simplifies configuration for users
...
🤔 hebasto reviewed a pull request: "rpc: Use untranslated error strings in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/30395#pullrequestreview-2169539317)
Post-merge ACK fa5b8920be041380fbfa4c7b443918637423d7a0.
(https://github.com/bitcoin/bitcoin/pull/30395#pullrequestreview-2169539317)
Post-merge ACK fa5b8920be041380fbfa4c7b443918637423d7a0.
💬 fjahr commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#discussion_r1672545632)
> nit in [e78cf83](https://github.com/bitcoin/bitcoin/commit/e78cf83beec2f508823803947bdc3d9bcb7c7217): Not sure why nbits matter for this test.
Yeah, I had just reused the helper method from this test but it's not needed indeed. I am using an empty constructor now.
> I think you can just write `CBlockIndex block{.nChainTx=..max()};`, replacing the first two lines.
I am unfamiliar with the `..max()` syntax and I am also getting a compiler error when I try to use a designated initializer
...
(https://github.com/bitcoin/bitcoin/pull/29656#discussion_r1672545632)
> nit in [e78cf83](https://github.com/bitcoin/bitcoin/commit/e78cf83beec2f508823803947bdc3d9bcb7c7217): Not sure why nbits matter for this test.
Yeah, I had just reused the helper method from this test but it's not needed indeed. I am using an empty constructor now.
> I think you can just write `CBlockIndex block{.nChainTx=..max()};`, replacing the first two lines.
I am unfamiliar with the `..max()` syntax and I am also getting a compiler error when I try to use a designated initializer
...
💬 fjahr commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#issuecomment-2220876912)
> I think you can drop the new name from the pull description. It is wrong right now (doesn't match the scripted diff), and if anyone wanted to find it, they can look at the scripted diff.
done
(https://github.com/bitcoin/bitcoin/pull/29656#issuecomment-2220876912)
> I think you can drop the new name from the pull description. It is wrong right now (doesn't match the scripted diff), and if anyone wanted to find it, they can look at the scripted diff.
done
💬 hodlinator commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2220906382)
> So going forward, I guess one can only assume to surely reproduce a fuzz crash by using the exact same stdlib (and compiler). For context there is also a compiler behavior mismatch discussed in #29043.
This is a strong counter-point. I guess other stdlib-differences like `std::unordered_map` implementations already made this true though.
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2220906382)
> So going forward, I guess one can only assume to surely reproduce a fuzz crash by using the exact same stdlib (and compiler). For context there is also a compiler behavior mismatch discussed in #29043.
This is a strong counter-point. I guess other stdlib-differences like `std::unordered_map` implementations already made this true though.
💬 sipa commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2220914013)
I don't think there are *any* `std::shuffle` in the fuzz tests, though?
It may make sense to add something like the old `Shuffle` back if we do need it, though.
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2220914013)
I don't think there are *any* `std::shuffle` in the fuzz tests, though?
It may make sense to add something like the old `Shuffle` back if we do need it, though.
💬 maflcko commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2220931147)
> I don't think there are _any_ `std::shuffle` in the fuzz tests, though?
I hope all code touched in this pull request is covered by fuzz tests. If not, then that should be fixed ;)
For example, `joinpsbts` is an RPC (not in the fuzz dir), but it is fuzzed, and it calls `std::shuffle`: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/rpc/rawtransaction.cpp.gcov.html#L1793
I don't have a strong feeling either way, because if an effort is made to have uniform fuzz behavior across compi
...
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2220931147)
> I don't think there are _any_ `std::shuffle` in the fuzz tests, though?
I hope all code touched in this pull request is covered by fuzz tests. If not, then that should be fixed ;)
For example, `joinpsbts` is an RPC (not in the fuzz dir), but it is fuzzed, and it calls `std::shuffle`: https://maflcko.github.io/b-c-cov/fuzz.coverage/src/rpc/rawtransaction.cpp.gcov.html#L1793
I don't have a strong feeling either way, because if an effort is made to have uniform fuzz behavior across compi
...
📝 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 */
*/ ```