💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2110820827)
Done.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2110820827)
Done.
💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2110819198)
Done.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2110819198)
Done.
💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2110820744)
Done.
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2110820744)
Done.
💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2110820382)
I've written a test based on this, which randomly adds dependencies but in a "directed" sense that never introduces cycles (rather than just has 2 specific scenarios).
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2110820382)
I've written a test based on this, which randomly adds dependencies but in a "directed" sense that never introduces cycles (rather than just has 2 specific scenarios).
💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#issuecomment-2915121835)
Hi @achow101 @maflcko @davidrobinsonau can you please check this once and merge it everything is fine.
(https://github.com/bitcoin/bitcoin/pull/31668#issuecomment-2915121835)
Hi @achow101 @maflcko @davidrobinsonau can you please check this once and merge it everything is fine.
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2915387816)
#31375 landed!
Still in draft waiting for https://github.com/bitcoin-core/libmultiprocess/pull/172
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2915387816)
#31375 landed!
Still in draft waiting for https://github.com/bitcoin-core/libmultiprocess/pull/172
💬 Sjors commented on issue "Stratum v2 via IPC Mining Interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/31098#issuecomment-2915399359)
The wrapper binary PR #31375 just landed, which is great progress!
The only big change left is to add bitcoin-node (aka `bitcoin -m`) to the release binaries, which is what #31802 does.
(https://github.com/bitcoin/bitcoin/issues/31098#issuecomment-2915399359)
The wrapper binary PR #31375 just landed, which is great progress!
The only big change left is to add bitcoin-node (aka `bitcoin -m`) to the release binaries, which is what #31802 does.
💬 i-am-yuvi commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2915403993)
Code Review ACK 93b07997e9a38523f5ab850aa32ca57983fd2552
I've made a review notes for this pr -> [here](https://github.com/i-am-yuvi/bitcoin-ibd/blob/master/30251.md)
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2915403993)
Code Review ACK 93b07997e9a38523f5ab850aa32ca57983fd2552
I've made a review notes for this pr -> [here](https://github.com/i-am-yuvi/bitcoin-ibd/blob/master/30251.md)
💬 maflcko commented on pull request "test: add MAX_DISCONNECTED_TX_POOL_BYTES, chainlimits coverage":
(https://github.com/bitcoin/bitcoin/pull/32516#issuecomment-2915446066)
re-ACK 84aa484d45e2fb3c1149941ef23779e4adb983d9 🚋
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 84aa484d45e2fb3c1149
...
(https://github.com/bitcoin/bitcoin/pull/32516#issuecomment-2915446066)
re-ACK 84aa484d45e2fb3c1149941ef23779e4adb983d9 🚋
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 84aa484d45e2fb3c1149
...
💬 hebasto commented on issue "build: use UCRT runtime for Windows (release) binaries":
(https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-2915451888)
@jeandudey
> I've sent a patchset a while ago when working on cross-base updating MinGW to 12.0.0: https://issues.guix.gnu.org/71630.
>
> Using `UCRT` will likely need a new target triplet for Guix though like MSYS2 does, e.g. `x86_64-w64-ucrt-mingw32`.
Trying to build `mingw-w64` with the deleted [`--with-default-msvcrt=msvcrt`](https://codeberg.org/guix/guix/src/commit/d1ec5be63e2d8521a6bad61e5c478319280623c6/gnu/packages/mingw.scm#L90) configure option results in a linker error:
```
x86_6
...
(https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-2915451888)
@jeandudey
> I've sent a patchset a while ago when working on cross-base updating MinGW to 12.0.0: https://issues.guix.gnu.org/71630.
>
> Using `UCRT` will likely need a new target triplet for Guix though like MSYS2 does, e.g. `x86_64-w64-ucrt-mingw32`.
Trying to build `mingw-w64` with the deleted [`--with-default-msvcrt=msvcrt`](https://codeberg.org/guix/guix/src/commit/d1ec5be63e2d8521a6bad61e5c478319280623c6/gnu/packages/mingw.scm#L90) configure option results in a linker error:
```
x86_6
...
💬 maflcko commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2111303331)
> Reading the undo data directly from storage (i.e. without re-serialization) results in ~3.2x requests per second, and even a bit less data sent over the network:
Hmm. I am not sure if this is representative of the end-to-end performance. Simply fetching the raw (compressed) data is missing the decompression overhead. The end product can probably not do much with the compressed data by itself, so the benchmark should include the decompression as well.
(https://github.com/bitcoin/bitcoin/pull/32540#discussion_r2111303331)
> Reading the undo data directly from storage (i.e. without re-serialization) results in ~3.2x requests per second, and even a bit less data sent over the network:
Hmm. I am not sure if this is representative of the end-to-end performance. Simply fetching the raw (compressed) data is missing the decompression overhead. The end product can probably not do much with the compressed data by itself, so the benchmark should include the decompression as well.
💬 fanquake commented on pull request "guix: accomodate migration to codeberg":
(https://github.com/bitcoin/bitcoin/pull/32439#issuecomment-2915516389)
Backported to `29.x` in #32589.
(https://github.com/bitcoin/bitcoin/pull/32439#issuecomment-2915516389)
Backported to `29.x` in #32589.
💬 stringintech commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2915572044)
> An alternative would be to not give these peers unlimited time but clear their block requests (so that these blocks can be requested from other peers) so that they wouldn't be disconnected but also wouldn't be able to stall us indefinitely.
This makes sense, as the PR tries to achieve a similar goal with initial headers sync timeouts in commit 64b956f.
(https://github.com/bitcoin/bitcoin/pull/32051#issuecomment-2915572044)
> An alternative would be to not give these peers unlimited time but clear their block requests (so that these blocks can be requested from other peers) so that they wouldn't be disconnected but also wouldn't be able to stall us indefinitely.
This makes sense, as the PR tries to achieve a similar goal with initial headers sync timeouts in commit 64b956f.
👍 hodlinator approved a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2874254446)
re-ACK 63854e8a81fdef3ca99ebd339db72563d053b9d0
While the code is made less straightforward due to optimizations, I think it's still worth doing it to minimize overhead from obfuscation.
Changes since first ACK (https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2852896828):
* Changed commit messages to include benchmark results from more recent GCC and newer vanilla instead of Apple-flavoured Clang.
* Modified unit test to add verification of obfuscation starting at non-ali
...
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2874254446)
re-ACK 63854e8a81fdef3ca99ebd339db72563d053b9d0
While the code is made less straightforward due to optimizations, I think it's still worth doing it to minimize overhead from obfuscation.
Changes since first ACK (https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2852896828):
* Changed commit messages to include benchmark results from more recent GCC and newer vanilla instead of Apple-flavoured Clang.
* Modified unit test to add verification of obfuscation starting at non-ali
...
📝 strmfos opened a pull request: "Replace dead gnome link notificator.cpp"
(https://github.com/bitcoin/bitcoin/pull/32629)
Hey team—noticed a dead link, replaced it with a working URL
https://developer.gnome.org/notification-spec/ - old link
https://developer.gnome.org/documentation/tutorials/notifications.html - new link
(https://github.com/bitcoin/bitcoin/pull/32629)
Hey team—noticed a dead link, replaced it with a working URL
https://developer.gnome.org/notification-spec/ - old link
https://developer.gnome.org/documentation/tutorials/notifications.html - new link
💬 furszy commented on pull request "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#issuecomment-2915854169)
Rebased, fixed a typo and added an extra commit deduplicating the wallet re-loading code.
> There are some typos reported, which look real: [#31423 (comment)](https://github.com/bitcoin/bitcoin/pull/31423#issuecomment-2520009159).
done thanks. It would be neat to have the bot outputting the typo commit hash too (just a small idea, maybe someone else wants to implement it).
(https://github.com/bitcoin/bitcoin/pull/31423#issuecomment-2915854169)
Rebased, fixed a typo and added an extra commit deduplicating the wallet re-loading code.
> There are some typos reported, which look real: [#31423 (comment)](https://github.com/bitcoin/bitcoin/pull/31423#issuecomment-2520009159).
done thanks. It would be neat to have the bot outputting the typo commit hash too (just a small idea, maybe someone else wants to implement it).
💬 furszy commented on pull request "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#discussion_r2111558553)
if your implementation works, we could replace the current sqlite `Files()` code for your code (using `sqlite3_filename_database` and `sqlite3_filename_journal` instead of the hardcoded path).
(https://github.com/bitcoin/bitcoin/pull/31423#discussion_r2111558553)
if your implementation works, we could replace the current sqlite `Files()` code for your code (using `sqlite3_filename_database` and `sqlite3_filename_journal` instead of the hardcoded path).
🤔 ismaelsadeeq reviewed a pull request: "random: Use modern Windows randomness functions"
(https://github.com/bitcoin/bitcoin/pull/32400#pullrequestreview-2874584557)
Post Merge Concept and Code review ACK 6b4bcc16234575108bb691c15c3532198d9bf98a
I found this issue orthogonally, and discovered that the fix has already being merged which is nice.
I verified that the implementation here matches the API https://learn.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom
(https://github.com/bitcoin/bitcoin/pull/32400#pullrequestreview-2874584557)
Post Merge Concept and Code review ACK 6b4bcc16234575108bb691c15c3532198d9bf98a
I found this issue orthogonally, and discovered that the fix has already being merged which is nice.
I verified that the implementation here matches the API https://learn.microsoft.com/en-us/windows/win32/api/bcrypt/nf-bcrypt-bcryptgenrandom
💬 hebasto commented on pull request "Replace dead gnome link notificator.cpp":
(https://github.com/bitcoin/bitcoin/pull/32629#discussion_r2111680585)
I don't see how the new link helps with understanding the `Notificator::notifyDBus()` function code Perhaps it's best to just remove the outdated comment?
(https://github.com/bitcoin/bitcoin/pull/32629#discussion_r2111680585)
I don't see how the new link helps with understanding the `Notificator::notifyDBus()` function code Perhaps it's best to just remove the outdated comment?
🤔 maflcko reviewed a pull request: "ci, iwyu: Treat warnings as errors for specific targets"
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-2874741428)
left some comments
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-2874741428)
left some comments