💬 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
...
💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1714936862)
How significant do we expect the `reconsider_pot` optimization to be? Locally my `LinearizePerIter*TxWorstCase` bench doesn't seem to do better after c68d26a0e97.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1714936862)
How significant do we expect the `reconsider_pot` optimization to be? Locally my `LinearizePerIter*TxWorstCase` bench doesn't seem to do better after c68d26a0e97.
🤔 glozow reviewed a pull request: "net: Clarify that m_addr_local is only set once"
(https://github.com/bitcoin/bitcoin/pull/30617#pullrequestreview-2234965570)
ACK fa6fe432075df5e0eceb1ccd85038159cc820ccc
(https://github.com/bitcoin/bitcoin/pull/30617#pullrequestreview-2234965570)
ACK fa6fe432075df5e0eceb1ccd85038159cc820ccc
💬 TheCharlatan commented on pull request "guix: bump time-machine to 7bf1d7aeaffba15c4f680f93ae88fbef25427252":
(https://github.com/bitcoin/bitcoin/pull/30609#issuecomment-2285750631)
Re https://github.com/bitcoin/bitcoin/pull/30609#pullrequestreview-2228473719
Riscv builds match as well.
(https://github.com/bitcoin/bitcoin/pull/30609#issuecomment-2285750631)
Re https://github.com/bitcoin/bitcoin/pull/30609#pullrequestreview-2228473719
Riscv builds match as well.
💬 0xawaz commented on issue "contrib: Automation for Bitcoin Full Node Deployment":
(https://github.com/bitcoin/bitcoin/issues/30638#issuecomment-2285753829)
> > **Ansible/Monitoring:** I agree, tooling preferences/ideologies are a thing now, I am agnostic, I adapt, my questions:
> > ```
> > * What tooling do you, Bitcoin core devs, and community support for system provisioning?
> >
> > * What tooling for monitoring?
> > ```
>
> Bitcoin Core is also agnostic, because it should work with any monitoring or provisioning solution out there out of the box with no further changes needed. (There are many out there, including graphana, or self-writt
...
(https://github.com/bitcoin/bitcoin/issues/30638#issuecomment-2285753829)
> > **Ansible/Monitoring:** I agree, tooling preferences/ideologies are a thing now, I am agnostic, I adapt, my questions:
> > ```
> > * What tooling do you, Bitcoin core devs, and community support for system provisioning?
> >
> > * What tooling for monitoring?
> > ```
>
> Bitcoin Core is also agnostic, because it should work with any monitoring or provisioning solution out there out of the box with no further changes needed. (There are many out there, including graphana, or self-writt
...