💬 HowHsu commented on pull request "index: Fix missing case in the comment in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2253156038)
All the cases are actually handled by this function, the thing is just when pindex_prev equals chain.Tip(), it is originally happened to be handled by `return chain.Next(chain.FindFork(pindex_prev));`, which is not for this case. So I split it out to be explicitly handled. Thus I guess it's hard to make a test for this.

💬 HowHsu commented on pull request "index: Fix missing case in the comment in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2253157922)
> All the cases are actually handled by this function, the thing is just when pindex_prev equals chain.Tip(), it is originally happened to be handled by `return chain.Next(chain.FindFork(pindex_prev));`, which is not for this case. So I split it out to be explicitly handled. Thus I guess it's hard to make a test for this.

All the cases are actually handled by this function, the thing is just when pindex_prev equals chain.Tip(), it is originally happened to be handled by return chain.Next(chai
...

💬 HowHsu commented on pull request "index: Fix missing case in the comment in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2253160852)
> seems you've found a code that isn't covered by tests - could you add a test case which showcases the need for this addition (such that if we revert this code the test fails)?

All the cases are actually handled by this function, the thing is just when pindex_prev equals chain.Tip(), it is originally happened to be handled by `return chain.Next(chain.FindFork(pindex_prev));`, which is not for this case. So I split it out to be explicitly handled. Thus I guess it's hard to make a test for thi
...

💬 l0rinc commented on pull request "index: Fix missing case in the comment in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2253162596)
(if you delete and repost instead of editing, we're getting multiple emails. if we need to modify it, we just cross out old version and add `edit:` to add the new part)

💬 HowHsu commented on pull request "index: Fix missing case in the comment in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2253164353)
> (if you delete and repost instead of editing, we're getting multiple emails. if we need to modify it, we just cross out old version and add `edit:` to add the new part)

Thanks for reminding.

💬 l0rinc commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#issuecomment-3153431055)
Code review reACK 6bf4f4a87889b0679c744b0a171c98a9386fe8c0

Rebased without other changes since last ACK

💬 HowHsu commented on pull request "index: remove unnecessary locater cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#issuecomment-3153433563)
> The commit message and PR description needs to be updated now. Also consider adding co-authors when someone else also contributed.

True. Done.

💬 l0rinc commented on pull request "index: remove unnecessary locater cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#issuecomment-3153442806)
There's still a typo in the title

👍 l0rinc approved a pull request: "index: remove unnecessary locater cleaning in BaseIndex::Init()"
(https://github.com/bitcoin/bitcoin/pull/32882#pullrequestreview-3086679876)
ACK ce7323ec635c389732e142c043f83868ddcefdb3

The commit message still sounds a bit unnatural and has a few typos, consider something like:
> index: remove unnecessary locator cleaning in BaseIndex::Init()
>
> Refactor BaseIndex::DB::ReadBestBlock() to return an optional value so that
callers don't need to rely on the locator itself to check if the call is
successful or not. This simplifies the code.

or just simply:
> refactor: remove redundant locator cleanup in BaseIndex::Init()
>
...

💬 maflcko commented on pull request "rpc: fix getpeerinfo ping duration unit docs":
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3153746243)
lgtm ACK 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff

💬 maflcko commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2253418093)
https://cirrus-ci.com/task/4521033710960640?logs=ci#L2066:

```
[11:45:25.309] test 2025-08-04T15:45:24.445408Z TestFramework (ERROR): Unexpected exception
[11:45:25.309] Traceback (most recent call last):
[11:45:25.309] File "/ci_container_base/test/functional/test_framework/test_framework.py", line 195, in main
[11:45:25.309] self.run_test()
[11:45:25.309]
...

💬 maflcko commented on pull request "tests: cover abortrescan() in-flight True path with dynamic-tail retry":
(https://github.com/bitcoin/bitcoin/pull/33131#discussion_r2253426578)
This is racy and will intermittently fail. You can test this by adding a time.sleep(...) before the abort.

🤔 janb84 reviewed a pull request: "rpc: fix getpeerinfo ping duration unit docs"
(https://github.com/bitcoin/bitcoin/pull/33133#pullrequestreview-3086979853)
ACK 1252eeb997df2eb12c33d92eb1a5c9d6643a67ff

Pr changes getpeerinfo documentation to return the correct unit base (seconds instead of milliseconds)

- Tested ✅
- Code review ✅

(Thanks for addressing the commit message !)

🚀 fanquake merged a pull request: "rpc: fix getpeerinfo ping duration unit docs"
(https://github.com/bitcoin/bitcoin/pull/33133)

💬 fanquake commented on pull request "rpc: fix getpeerinfo ping duration unit docs":
(https://github.com/bitcoin/bitcoin/pull/33133#issuecomment-3154144457)
Backported to `29.x` in #33074.

💬 cedwies commented on pull request "tests: cover abortrescan() in-flight True path with dynamic-tail retry":
(https://github.com/bitcoin/bitcoin/pull/33131#discussion_r2253674701)
Are you referring to the scan finishing between "scanning=True" and "abortrescan()"?
I have thought about this, but I am not sure how to best solve the issue. If we saw "scanning=True" but "abortrescan()=False" we could assume a race and just try again (e.g. up to 5 times). But maybe this blows up the test suite?

💬 maflcko commented on pull request "tests: cover abortrescan() in-flight True path with dynamic-tail retry":
(https://github.com/bitcoin/bitcoin/pull/33131#discussion_r2253688326)
> Are you referring to the scan finishing between "scanning=True" and "abortrescan()"?

yes


> and just try again (e.g. up to 5 times).

This will still fail, if it fails 5 times

🚀 fanquake merged a pull request: "allocators: Apply manual ASan poisoning to `PoolResource`"
(https://github.com/bitcoin/bitcoin/pull/32581)

🤔 danielabrozzoni reviewed a pull request: "cli: return local services in -netinfo"
(https://github.com/bitcoin/bitcoin/pull/31886#pullrequestreview-3087515075)
reACK 721a051320f2c10d2e9c89c985f180da81d64dca

👍 dergoegge approved a pull request: "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts"
(https://github.com/bitcoin/bitcoin/pull/32473#pullrequestreview-3087710077)
Code review ACK fb7e9decf3f12ebae786e0cecf2f24a91c6e9d4c