💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714564547)
re: https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712563888
Thanks for the links. I don't think I realized this change was made in the other PR, and I think it is not a good change.
I don't see a reason somebody shouldn't be able to run `RANDOM_CTX_SEED=12345 ./test_bitcoin` conveniently from a command line or script without padding the seed with 59 0's. It seems like this makes it pointlessly difficult to specify a random seed and I don't get the motivation behind it.
If w
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714564547)
re: https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712563888
Thanks for the links. I don't think I realized this change was made in the other PR, and I think it is not a good change.
I don't see a reason somebody shouldn't be able to run `RANDOM_CTX_SEED=12345 ./test_bitcoin` conveniently from a command line or script without padding the seed with 59 0's. It seems like this makes it pointlessly difficult to specify a random seed and I don't get the motivation behind it.
If w
...
💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714524098)
re: https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712444215
> An alternative would be to, as @maflcko initially [suggested](https://github.com/bitcoin/bitcoin/pull/30569/#discussion_r1700712603), just have this be a parameter of `base_blob::FromHex()`, but that feels less elegant to me, still. What do you think?
Yes I think that suggestion would be a lot better! It's hard to imagine this function being used for anything other than as a preprocessor to `FromHex`. And I think it
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714524098)
re: https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712444215
> An alternative would be to, as @maflcko initially [suggested](https://github.com/bitcoin/bitcoin/pull/30569/#discussion_r1700712603), just have this be a parameter of `base_blob::FromHex()`, but that feels less elegant to me, still. What do you think?
Yes I think that suggestion would be a lot better! It's hard to imagine this function being used for anything other than as a preprocessor to `FromHex`. And I think it
...
💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714528877)
> > The number is not just rejected when it is too long but also if it has whitespace prefixes or suffixes or trailing non-hex characters. These would have been ignored previously.
>
> Is that so? It seems to me that `IsHexNumber()` would have returned false for all of those cases, which is why [802374b](https://github.com/bitcoin/bitcoin/commit/802374b4355bd1dec7a88bba6287c55f935699fe) is a refactor commit?
Oh, sorry I forgot about the previous call. So this commit message is perfectly ac
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714528877)
> > The number is not just rejected when it is too long but also if it has whitespace prefixes or suffixes or trailing non-hex characters. These would have been ignored previously.
>
> Is that so? It seems to me that `IsHexNumber()` would have returned false for all of those cases, which is why [802374b](https://github.com/bitcoin/bitcoin/commit/802374b4355bd1dec7a88bba6287c55f935699fe) is a refactor commit?
Oh, sorry I forgot about the previous call. So this commit message is perfectly ac
...
💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714681251)
> Thanks for the links. I don't think I realized this change was made in the other PR, and I think it is not a good change.
>
> I don't see a reason somebody shouldn't be able to run `RANDOM_CTX_SEED=12345 ./test_bitcoin` conveniently from a command line or script without padding the seed with 59 0's. It seems like this makes it pointlessly difficult to specify a random seed and I don't get the motivation behind it.
No strong opinion, but my thinking was that the seed would at most be copy
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714681251)
> Thanks for the links. I don't think I realized this change was made in the other PR, and I think it is not a good change.
>
> I don't see a reason somebody shouldn't be able to run `RANDOM_CTX_SEED=12345 ./test_bitcoin` conveniently from a command line or script without padding the seed with 59 0's. It seems like this makes it pointlessly difficult to specify a random seed and I don't get the motivation behind it.
No strong opinion, but my thinking was that the seed would at most be copy
...
💬 maflcko commented on pull request "test: update satoshi_round function":
(https://github.com/bitcoin/bitcoin/pull/29566#issuecomment-2285377167)
review ACK ec317bc44b0cb089419d809b5fef38ecb9423051
(https://github.com/bitcoin/bitcoin/pull/29566#issuecomment-2285377167)
review ACK ec317bc44b0cb089419d809b5fef38ecb9423051
💬 maflcko commented on pull request "doc: add missing "testnet4" network string in RPC/init help texts":
(https://github.com/bitcoin/bitcoin/pull/30642#discussion_r1714711575)
Would it make sense to have a `#define LIST_CHAIN_NAMES "main, test, testnet4, signet, regtest"` in `src/chainparamsbase.h`? Otherwise, the lines of code will have to be touched again when testnet3 is removed.
```suggestion
{RPCResult::Type::STR, "chain", "current network name (" LIST_CHAIN_NAMES ")"},
```
(https://github.com/bitcoin/bitcoin/pull/30642#discussion_r1714711575)
Would it make sense to have a `#define LIST_CHAIN_NAMES "main, test, testnet4, signet, regtest"` in `src/chainparamsbase.h`? Otherwise, the lines of code will have to be touched again when testnet3 is removed.
```suggestion
{RPCResult::Type::STR, "chain", "current network name (" LIST_CHAIN_NAMES ")"},
```
👍 maflcko approved a pull request: "doc: add missing "testnet4" network string in RPC/init help texts"
(https://github.com/bitcoin/bitcoin/pull/30642#pullrequestreview-2234570982)
review ACK 701530045553f2b9671a3fffea301bf4dc954514
(https://github.com/bitcoin/bitcoin/pull/30642#pullrequestreview-2234570982)
review ACK 701530045553f2b9671a3fffea301bf4dc954514
💬 maflcko commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1714713004)
> - #include <cassert> // lines 9-9
This is wrong. It is used.
Removed the other.
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1714713004)
> - #include <cassert> // lines 9-9
This is wrong. It is used.
Removed the other.
💬 maflcko commented on pull request "coins: Add move operations to CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2285409839)
> Sonar
They are mostly just a clang-tidy wrapper, so I wonder if there is a clang-tidy warning that can be used here instead.
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2285409839)
> Sonar
They are mostly just a clang-tidy wrapper, so I wonder if there is a clang-tidy warning that can be used here instead.
💬 paplorinc commented on pull request "coins: Add move operations to CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2285442194)
> I wonder if there is a clang-tidy warning
I have installed Sonar locally, reproduded the warning first, it doesn't appear anymore afer the change.
> Don't we also need to add move constructor to Coin?
It seems to me that the `Coin` class already has an explicit move constructor that moves the `CTxOut` member - and the rest are trivially movable (i.e. bit-copy of `unsigned int` and `uint32_t`).
```C++
Coin(CTxOut&& outIn, int nHeightIn, bool fCoinBaseIn) : out(std::move(outIn)),
...
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2285442194)
> I wonder if there is a clang-tidy warning
I have installed Sonar locally, reproduded the warning first, it doesn't appear anymore afer the change.
> Don't we also need to add move constructor to Coin?
It seems to me that the `Coin` class already has an explicit move constructor that moves the `CTxOut` member - and the rest are trivially movable (i.e. bit-copy of `unsigned int` and `uint32_t`).
```C++
Coin(CTxOut&& outIn, int nHeightIn, bool fCoinBaseIn) : out(std::move(outIn)),
...
👋 paplorinc's pull request is ready for review: "coins: Add move operations to CCoinsCacheEntry"
(https://github.com/bitcoin/bitcoin/pull/30643)
(https://github.com/bitcoin/bitcoin/pull/30643)
💬 maflcko commented on issue "implicit-integer-sign-change in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2285605841)
Personally, what I find a lot more interesting than the discussion whether or not a metadata field should exist, is the evaluation of how a fuzz engine can find the initially reported bug at all.
Right now, I couldn't get a fuzz engine to find it, regardless of what I do.
(The initial finding was due to a manually written fuzz input generator, but that seems suboptimal, because it is difficult to scale and it would be better if a fuzz engine could find the bug by itself).
I think it is
...
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2285605841)
Personally, what I find a lot more interesting than the discussion whether or not a metadata field should exist, is the evaluation of how a fuzz engine can find the initially reported bug at all.
Right now, I couldn't get a fuzz engine to find it, regardless of what I do.
(The initial finding was due to a manually written fuzz input generator, but that seems suboptimal, because it is difficult to scale and it would be better if a fuzz engine could find the bug by itself).
I think it is
...
📝 maflcko opened a pull request: "fuzz: Faster utxo_snapshot fuzz target"
(https://github.com/bitcoin/bitcoin/pull/30644)
Two commits to speed up unit and fuzz tests.
Can be tested by running the fuzz target and looking at the time it took, or by looking at the flamegraph. For example:
```
FUZZ=utxo_snapshot perf record -g --call-graph dwarf ./src/test/fuzz/fuzz -runs=100
hotspot ./perf.data
(https://github.com/bitcoin/bitcoin/pull/30644)
Two commits to speed up unit and fuzz tests.
Can be tested by running the fuzz target and looking at the time it took, or by looking at the flamegraph. For example:
```
FUZZ=utxo_snapshot perf record -g --call-graph dwarf ./src/test/fuzz/fuzz -runs=100
hotspot ./perf.data
💬 maflcko commented on issue "implicit-integer-sign-change in ActivateSnapshot":
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2285656510)
> Also, the fuzz target is slow, so that should probably be fixed first.
Initial attempt in https://github.com/bitcoin/bitcoin/pull/30644
(https://github.com/bitcoin/bitcoin/issues/30514#issuecomment-2285656510)
> Also, the fuzz target is slow, so that should probably be fixed first.
Initial attempt in https://github.com/bitcoin/bitcoin/pull/30644
💬 maflcko commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#issuecomment-2285658238)
Force pushed for iwyu, and to add a new (only half-related) commit.
(https://github.com/bitcoin/bitcoin/pull/29071#issuecomment-2285658238)
Force pushed for iwyu, and to add a new (only half-related) commit.
💬 fanquake commented on pull request "coins: Add move operations to CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2285667442)
> reproduded the warning first, it doesn't appear anymore afer the change.
Can you add any relevant output to the PR description.
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2285667442)
> reproduded the warning first, it doesn't appear anymore afer the change.
Can you add any relevant output to the PR description.
💬 dergoegge commented on pull request "fuzz: Faster utxo_snapshot fuzz target":
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2285676482)
What is the expected speed up?
(https://github.com/bitcoin/bitcoin/pull/30644#issuecomment-2285676482)
What is the expected speed up?
👍 hodlinator approved a pull request: "refactor: Remove Span operator==, Use std::ranges::equal"
(https://github.com/bitcoin/bitcoin/pull/29071#pullrequestreview-2234894161)
Untested Code Review re-ACK fad0cf6
`git range-diff master fa69fba fad0cf6` on shows `#include` changes and new fad0cf6f26 commit.
`git show fad0cf6` shows use of `std::equal` overload with 3 iterators being replaced with the *generally* safer `std::ranges::equal` (not really an issue in this specific case as we are comparing pairs of the same type `MessageStartChars = std::array<uint8_t, 4>`).
(https://github.com/bitcoin/bitcoin/pull/29071#pullrequestreview-2234894161)
Untested Code Review re-ACK fad0cf6
`git range-diff master fa69fba fad0cf6` on shows `#include` changes and new fad0cf6f26 commit.
`git show fad0cf6` shows use of `std::equal` overload with 3 iterators being replaced with the *generally* safer `std::ranges::equal` (not really an issue in this specific case as we are comparing pairs of the same type `MessageStartChars = std::array<uint8_t, 4>`).
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1714902269)
Indeed, this should not have been a while loop. I reproduced the infinite loop here.
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1714902269)
Indeed, this should not have been a while loop. I reproduced the infinite loop here.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2285719575)
Thank you for the questions and kicking this discussion off @ryanofsky! I'll update the PR description with a better motiviation re. C vs C++ header, but will also try to answer your questions here.
> This seems to offer a lot of nice features, but can you explain the tradeoffs of wrapping the C++ interface in C instead of using C++ from rust directly? It seems like having a C middle layer introduces a lot of boilerplate, and I'm wondering if it is really necessary. For example it seems like
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2285719575)
Thank you for the questions and kicking this discussion off @ryanofsky! I'll update the PR description with a better motiviation re. C vs C++ header, but will also try to answer your questions here.
> This seems to offer a lot of nice features, but can you explain the tradeoffs of wrapping the C++ interface in C instead of using C++ from rust directly? It seems like having a C middle layer introduces a lot of boilerplate, and I'm wondering if it is really necessary. For example it seems like
...