💬 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.
👍 TheCharlatan approved a pull request: "fuzz: Faster utxo_snapshot fuzz target"
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2235025267)
Nice, ACK fa48ded21168740640ff9d6e5b413a322d58b114
I'd re-ack quickly if you want to fix the other includes as well.
(https://github.com/bitcoin/bitcoin/pull/30644#pullrequestreview-2235025267)
Nice, ACK fa48ded21168740640ff9d6e5b413a322d58b114
I'd re-ack quickly if you want to fix the other includes as well.
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1715013521)
Fixed (in the same way as `waitforblock`)
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1715013521)
Fixed (in the same way as `waitforblock`)
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1715013631)
Fixed, though in a slightly different way.
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1715013631)
Fixed, though in a slightly different way.
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1715013711)
Done
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1715013711)
Done
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2285847403)
Re https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1713879417
> it looks like this will wait 1000ms+1000ms instead of 1000ms+500ms
Indeed it seems to pick the wrong wait time after the first inner loop. If fixed that and the overflow, though in a slightly different way than the AI overlord suggested.
I also added a `g_best_block == uint256()` check because it starts 0 until the first `UpdateTip()`.
Changed current_tip argument to `uint256` instead of `BlockRef`.
I also
...
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2285847403)
Re https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1713879417
> it looks like this will wait 1000ms+1000ms instead of 1000ms+500ms
Indeed it seems to pick the wrong wait time after the first inner loop. If fixed that and the overflow, though in a slightly different way than the AI overlord suggested.
I also added a `g_best_block == uint256()` check because it starts 0 until the first `UpdateTip()`.
Changed current_tip argument to `uint256` instead of `BlockRef`.
I also
...