💬 HowHsu commented on pull request "truc: optimize the in package relation calculation":
(https://github.com/bitcoin/bitcoin/pull/33062#issuecomment-3153322424)
> My understanding is that this isn't a bottleneck, optimizing it without a benchmark or an easily reproducible usecase seems like it's a solution begging for a problem - I speak from personal experience, that's how I started as well :)
>
> I left a few comments while reviewing anyway, but without a concept ack on the idea itself from those who are more familiar with it, it likely won't get merged.

Thanks, l0rinc. I'll leave this patch alone for now.

💬 HowHsu commented on pull request "index: remove unnecessary locater cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#issuecomment-3153348125)
> ACK [1f678d8](https://github.com/bitcoin/bitcoin/commit/1f678d832b1196ca7d7c2ff7e70d2172ae8c90b6)
>
> Recreated and retested it locally. It's a similar change to #33042, we have a few more places which should be cleanup up like this.
>
> I think this already makes the code slightly simpler, but alternatively, you could return an optional and not rely output params and nullness
>
> Details

Thanks, l0rinc. I've update the code as what you suggested

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

💬 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