Bitcoin Core Github
44 subscribers
121K links
Download Telegram
💬 ryanofsky commented on pull request "[IBD] Tracking PR for speeding up Initial Block Download":
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-2718577789)
Thanks for creating this. This should make it easier to navigate the other PRs and discuss the overall topic of IBD performance and benchmarking without needing to necessarily repeat it in the individual PRs.

Would be useful to have concept ACKs/NACKs here from others who know more about performance and benchmarking. But from from what I can tell the individual optimizations do not seem very complicated and seem like they should be justified.

---

One suggestion for the PR description ab
...
🤔 musaHaruna reviewed a pull request: "test: Use rpc_deprecated only for testing deprecation"
(https://github.com/bitcoin/bitcoin/pull/31977#pullrequestreview-2679357968)
Concept ACK [b1c80fa](https://github.com/bitcoin/bitcoin/commit/b1c80fab0c026a40f2901958abdeedb9dd36c30d)
New to this test, please correct me know if am wrong, after checking the history of the file, it seems that it has primarily been used to test deprecated RPCs both with and without the -deprecatedrpc flag. This change proposes to focus exclusively on testing the behavior of deprecated RPCs without the flag, as other tests already cover the cases where the -deprecatedrpc flag is used.
💬 maflcko commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1992051054)
Seems a bit odd to skip rescan when all timestamps are negative. Previously, this would rescan the full chain.
⚠️ Sjors opened an issue: "Wallet migration includes rescan"
(https://github.com/bitcoin/bitcoin/issues/32045)
Users may want to migrate legacy wallets they haven't used in years. The migration itself is typically very quick, but it's followed by a rescan.

Depending on whether the user has `-blockfilterindex` enabled, this rescan takes somewhere between long and really really long.

There's no visual indication, only the logs show what's going on:

<img width="383" alt="Image" src="https://github.com/user-attachments/assets/da911b44-8052-4b90-aa16-67625e550e3e" />

Ideally the migration should finish an
...
👋 l0rinc's pull request is ready for review: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144)
💬 maflcko commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-2718738419)
Forgot to mention it, but if Cirrus is replaced, it may be good to also update DrahtBot to properly assign the "CI failed" label, so that it is easy to monitor for (intermittent) CI failures via https://github.com/bitcoin/bitcoin/pulls?q=is%3Aopen+label%3A%22CI+failed%22+sort%3Aupdated-desc++is%3Apr+-label%3A%22Needs+rebase%22++
📝 glozow opened a pull request: "[29.x] bump to v29.0rc1"
(https://github.com/bitcoin/bitcoin/pull/32046)
- "backport" #32041
- bump version to v29.0rc1
- generate manpages
- add example bitcoin.conf
- add release-notes.md pointing to wiki
👍 hebasto approved a pull request: "[29.x] bump to v29.0rc1"
(https://github.com/bitcoin/bitcoin/pull/32046#pullrequestreview-2679517058)
ACK 628e4f59b8784fcde2de176fb98260e71def18ec, I have reviewed the code and it looks OK.
💬 glozow commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2718778336)
concept ACK, seems rfm (after a rebase?)
💬 hebasto commented on pull request "[29.x] bump to v29.0rc1":
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2718813885)
> ACK [628e4f5](https://github.com/bitcoin/bitcoin/commit/628e4f59b8784fcde2de176fb98260e71def18ec), I have reviewed the code and it looks OK.

Revoked. Man pages for `bitcoind` and `bitcoin-qt` miss "ZeroMQ notification options"
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1992120827)
Good catch. This is wrong, but should be harmless here, albeit highly confusing. Fixed.

Should be trivial to re-ack with `git range-diff bitcoin-core/master A B`
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1992120962)
Not sure what you mean with "retain". In current master there is no static extent, as this is using `Span`.

Also, streams don't make use of static extends at all. See also https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1929225185:

> Presumably some of the streams could benefit from static extents, but that's waaay overkill for here.

So my preference would be to not mix refactoring with questionable performance improvements.
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1992121110)
> which seems to eliminate the need for

Not sure. Even if this was the only place that makes use of this, for consistency it should be kept anyway. Otherwise, someone will have to touch this file again to add it back once it is used.
💬 glozow commented on pull request "[29.x] bump to v29.0rc1":
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2718842557)
Great, fixed.
💬 hebasto commented on pull request "[29.x] bump to v29.0rc1":
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2718855202)
> > ACK [628e4f5](https://github.com/bitcoin/bitcoin/commit/628e4f59b8784fcde2de176fb98260e71def18ec), I have reviewed the code and it looks OK.
>
> Revoked. Man pages for `bitcoind` and `bitcoin-qt` miss "ZeroMQ notification options"

My apologies for not checking all stuff at once, but `examples/bitcoin.conf` also lacks "ZeroMQ notification options".
💬 glozow commented on pull request "[29.x] bump to v29.0rc1":
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2718857240)
Thanks @hebasto!
👍 hebasto approved a pull request: "[29.x] bump to v29.0rc1"
(https://github.com/bitcoin/bitcoin/pull/32046#pullrequestreview-2679617137)
ACK 47e2fa86dc5433852fd9e5050a23de2accfdca8d.
🚀 ryanofsky merged a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283)
👍 ryanofsky approved a pull request: "[IBD] flush UXTOs in bigger batches"
(https://github.com/bitcoin/bitcoin/pull/31645#pullrequestreview-2679668648)
Code review ACK 868413340f8d6058d74186b65ac3498d6b7f254a

> If that spike exceeds the memory the process can allocate it causes a crash, at a particularly bad time (may require a replay to fix, which may be slower than just reprocessing the blocks).

It is difficult for me to have a sense of how safe this change is but I'd hope we are not currently pushing systems so close to the edge that using an extra 48mb will cause them to start crashing. This does seem like a nice performance improvmen
...