💬 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
...
💬 0xawaz commented on issue "contrib: Automation for Bitcoin Full Node Deployment":
(https://github.com/bitcoin/bitcoin/issues/30638#issuecomment-2285762113)
>
> * **Docker**
>
> * Since we all agree that we need an official docker image for Bitcoin Core, what all PR attempts fail to consider?
> * My understanding is Dockerfile maintenance is one of the main concerns, could you elaborate more? Do you need more hands in the team or do you need us to adapt to the Bitcoin Core build environment and CI/CD process?
> * To be able to succeed where previous attempts failed, I will need to dig more in the code, CI/CD workflows, and tricky blind
...
(https://github.com/bitcoin/bitcoin/issues/30638#issuecomment-2285762113)
>
> * **Docker**
>
> * Since we all agree that we need an official docker image for Bitcoin Core, what all PR attempts fail to consider?
> * My understanding is Dockerfile maintenance is one of the main concerns, could you elaborate more? Do you need more hands in the team or do you need us to adapt to the Bitcoin Core build environment and CI/CD process?
> * To be able to succeed where previous attempts failed, I will need to dig more in the code, CI/CD workflows, and tricky blind
...
💬 hodlinator commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714952020)
I didn't previously realize that this PR breaks use-facing compatibility wrt `0x` prefixes in args. Would also like to see such cases be filtered through `TrySanitizeHexNumber` before going into `FromHex` so that level of compatibility is maintained.
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714952020)
I didn't previously realize that this PR breaks use-facing compatibility wrt `0x` prefixes in args. Would also like to see such cases be filtered through `TrySanitizeHexNumber` before going into `FromHex` so that level of compatibility is maintained.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2285769580)
`2b478bf332...11364e2e58`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2285769580)
`2b478bf332...11364e2e58`: rebase due to conflicts
🤔 hodlinator requested changes to a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2234995444)
Downgrade to Concept ACK 855784d3a0026414159acc42fceeb271f8a28133 in light of https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714952020
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2234995444)
Downgrade to Concept ACK 855784d3a0026414159acc42fceeb271f8a28133 in light of https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714952020
🚀 glozow merged a pull request: "net: Clarify that m_addr_local is only set once"
(https://github.com/bitcoin/bitcoin/pull/30617)
(https://github.com/bitcoin/bitcoin/pull/30617)
🤔 glozow reviewed a pull request: "doc: add missing "testnet4" network string in RPC/init help texts"
(https://github.com/bitcoin/bitcoin/pull/30642#pullrequestreview-2235009746)
ACK 701530045553f2b9671a3fffea301bf4dc954514
(https://github.com/bitcoin/bitcoin/pull/30642#pullrequestreview-2235009746)
ACK 701530045553f2b9671a3fffea301bf4dc954514
💬 fjahr commented on pull request "validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2285794512)
> If a few more people can review this in the next day or so, I believe we can have this in for 28.0.
I have pinged a few more people for (re-)reviews and testing but please give it until the end of the week because testing the full flow can take a while depending on what machine you are doing it on or what connection speed you have.
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2285794512)
> If a few more people can review this in the next day or so, I believe we can have this in for 28.0.
I have pinged a few more people for (re-)reviews and testing but please give it until the end of the week because testing the full flow can take a while depending on what machine you are doing it on or what connection speed you have.