💬 maflcko commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-3048010967)
ref https://github.com/bitcoin/bitcoin/pull/32896

💬 maflcko commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-3048021752)
> PTAL

Not sure what you are asking for. Feedback has been provided, and the burden to address it and reply to it is on the pull request author. I can't make it go away by looking at it more.

Apart from the prior feedback, you'd also have to squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits into atomic units (https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches)

💬 maflcko commented on pull request "wallet: Prepare for future upgrades by recording versions of last client to open and decrypt":
(https://github.com/bitcoin/bitcoin/pull/32895#issuecomment-3048078577)
I wonder if this can be uncoupled from `CLIENT_VERSION`, because the coupling is confusing and makes testing harder:

* It is confusing, because switching between versions that kept the wallet code the same will pretend that there was a relevant upgrade/downgrade, even though the wallet behavior is exactly identical.
* `CLIENT_VERSION` on the development branch is changed every 6 months, not when a new wallet feature was added that needs to deal with un-upgraded material. I know that running
...

💬 maflcko commented on pull request "doc: fix `BlockConnected` incorrect comment":
(https://github.com/bitcoin/bitcoin/pull/32893#issuecomment-3048091064)
lgtm ACK 4e69aa5701a2dad3805ea26718e6a406adb8b748

🚀 fanquake merged a pull request: "doc: fix `BlockConnected` incorrect comment"
(https://github.com/bitcoin/bitcoin/pull/32893)

💬 maflcko commented on pull request "test: less ambiguous error if bitcoind is missing":
(https://github.com/bitcoin/bitcoin/pull/32921#discussion_r2191997393)
```suggestion
raise AssertionError("At least one binary is missing, did you compile?")
```

there are more binaries that could be missing, than just bitcoind?

💬 maflcko commented on pull request "Cache m_cached_finished_ibd where SetTip is called.":
(https://github.com/bitcoin/bitcoin/pull/32885#issuecomment-3048207836)
tight polling of is_ibd seems like a mistake in the first place, so i am not sure if this is something to optimize for.

Looking at the remaining call sites of the ibd check, most have cs_main already, so they won't be affected by this? The remaining ones (I only found `MaybeSendFeefilter`), if they are relevant, could either re-order their code to call it less often, or use cache the bool themselves? For the gui, see also https://github.com/bitcoin/bitcoin/issues/17145

💬 maflcko commented on pull request "rest: replace `rf_names[0].rf` by `RESTResponseFormat::UNDEF` for code clarity":
(https://github.com/bitcoin/bitcoin/pull/32884#issuecomment-3048247912)
lgtm ACK 6d19815cd44031ff2b45fc9532f579cd81c62749

💬 rkrux commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2192081742)
In 2923087479cce6117aa3f88a5476942d4af757b9 "refactor: use PSBTOptions for filling and signing"

`PSBTOptions` seems quite generic to me, maybe `PSBTFillingOptions`? The doc above also states something similar.

🤔 rkrux reviewed a pull request: "refactor: use options struct for signing and PSBT operations"
(https://github.com/bitcoin/bitcoin/pull/32876#pullrequestreview-2996953525)
Concept ACK 99e7ec91322abc809d343a83b31d82307f827f7f

I, too, usually prefer grouping together related items in a `struct`. In favour of `PSBTOptions` grouping as ISTM that it cleans up the code a bit. `SignOptions` could also prove to be helpful with the incoming changes in future PRs.

🤔 rkrux reviewed a pull request: "wallet: Remove watchonly behavior and isminetypes"
(https://github.com/bitcoin/bitcoin/pull/32523#pullrequestreview-2996982345)
I believe that the PR title and more importantly the PR description should be updated to remove the stuff related to watch-only as that code removal was done in another PR. The last paragraph of the description can be kept.

💬 rkrux commented on pull request "wallet: remove dead code in legacy wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32758#discussion_r2192124748)
Dropped the commit.

📝 Sjors opened a pull request: "test: use notarized v28.2 binaries and fix macOS detection"
(https://github.com/bitcoin/bitcoin/pull/32922)
Since https://github.com/bitcoin/bitcoin/pull/31407 guix builds are signed and notarized. This was included in v29 and backported to v28.

This PR bumps the v28.0 previous release binary to v28.2 and adjust the test that uses it. Additionally it no longer manually code signs binaries >= v28.2.

While testing on an M4 mac and redownloading all the binaries, I noticed that `platform == "arm64-apple-darwin"` doesn't actually work. So the first commit switches this to use `platform.system().lowe
...

💬 Sjors commented on pull request "test: less ambiguous error if bitcoind is missing":
(https://github.com/bitcoin/bitcoin/pull/32921#discussion_r2192218937)
Done

💬 Sjors commented on pull request "test: less ambiguous error if bitcoind is missing":
(https://github.com/bitcoin/bitcoin/pull/32921#issuecomment-3048496358)
If you're testing this on Apple Silicon, you may also need #32922.

💬 Sjors commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2192233867)
Renamed to `PSBTFillOptions`

💬 maflcko commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#discussion_r2192252386)
the code is still wrong. Usually, it doesn't make sense to compare two values of completely unrelated types and a type-safe language would prevent this class of issues.

Also, have you tested this code branch?

💬 Sjors commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#discussion_r2192291415)
Oh oops, I did test it, but then broke it when switching the commit order.

💬 Sjors commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#issuecomment-3048632834)
cc @kdmukai

💬 Sjors commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2192333715)
1a1b478ca31be1f670754a47da17863271e46b7b: missed one occurance of `platform`, see #32922