💬 pablomartin4btc commented on pull request "wallet: Remove `CWallet::nWalletVersion` and several related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2207617532)
I'd keep it for backwards compatibility.
`{RPCResult::Type::NUM, "walletversion", "(DEPRECATED) only related to unsupported legacy wallet, returns the latest version 169900 for backwards compatibility"},`

💬 pablomartin4btc commented on pull request "wallet: Remove `CWallet::nWalletVersion` and several related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2207618002)
Same as above, please keep it for backwards compatibility:

obj.pushKV("walletversion", 169900);

(or a `const` somewhere if we need that number for a little while)

💬 pablomartin4btc commented on pull request "wallet: Remove `CWallet::nWalletVersion` and several related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2207619247)
```suggestion
```
I had a chat with @achow101 about it yesterday, she commented to me that the "_hd chain split versioning is stored in CHDChain too_" so we can get rid of `FEATURE_HD_SPLIT` and the the entire `enum` there.

💬 pablomartin4btc commented on pull request "wallet: Remove `CWallet::nWalletVersion` and several related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2207624247)
This can be also removed, as mentioned above in `LegacyDataSPKM::MigrateToDescriptor()`, the hd split versioning can be obtained from the `CHDCHAIN`.

💬 pablomartin4btc commented on pull request "wallet: Remove `CWallet::nWalletVersion` and several related functions":
(https://github.com/bitcoin/bitcoin/pull/32977#discussion_r2207625886)
Same as above, we can keep it for another release until it gets removed completely.

🤔 pablomartin4btc reviewed a pull request: "wallet: Remove `CWallet::nWalletVersion` and several related functions"
(https://github.com/bitcoin/bitcoin/pull/32977#pullrequestreview-3020592257)
Concept ACK

I'd clarify in the description that this PR is based on top of #32944.

The title of this PR could be: "_wallet: Remove wallet version and several legacy related functions_.

I've left a few comments.

🤔 rkrux reviewed a pull request: "test: refactor: overhaul block hash determination for `CBlock{,Header}` objects"
(https://github.com/bitcoin/bitcoin/pull/32868#pullrequestreview-3020605701)
utACK 50b41bcfbfd93f103cf6e2af1999c43947bc3f40

This is the followup to the similar `CTransaction` cleanup PR. As I realised in the previous PR, this is a significant improvement in code readability and helps in getting rid of sneaky bugs too.

💬 PeterWrighten commented on pull request "wallet: Allow read-only database access for info and dump commands":
(https://github.com/bitcoin/bitcoin/pull/32685#issuecomment-3073846077)
> Re: [Merge branch 'master' into wallet-readonly-access](https://github.com/bitcoin/bitcoin/pull/32685/commits/fa0699eaffee914aded42fce21beffdebf41b1b6)
>
> I don't think merging master in the PR branch is preferred, instead rebasing is done. https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes

I have rebased but it seems one ci failed because of timeout issue. To confirm this issue did't fail accidentally, I request to rerun it.

💬 mzumsande commented on pull request "rpc,net: Add getpeerbyid and listpeersbyids RPCs":
(https://github.com/bitcoin/bitcoin/pull/32972#issuecomment-3073944718)
I also prefer the current approach from #32741 to this - while users can always just filter `getpeerinfo` output with jq or other tools I can see how that is inconvenient for some - but introducing multiple new RPCs for this seems overkill to me.

💬 fanquake commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2207734166)
> Is xz really necessary?

It's currently pulled in as a cmake dep, but it also seems fine to be explicit.

> and install samurai.

Yea, I think we should use `samurai` here.

👍 pinheadmz approved a pull request: "net: Fix Discover() not running when using -bind=0.0.0.0:port"
(https://github.com/bitcoin/bitcoin/pull/32757#pullrequestreview-3020667324)
ACK ad3f60b0ea59dbbe7aedcfdcca1caddefffe6976

Built and tested on macos/arm64 and debian/x86. Reviewed code changes which are minimal, this PR searches bind settings for 0.0.0.0 specifically to call Discover() which attempts to add local addresses for peer advertising. Also adds test coverage using `1.1.1.1` and `2.2.2.2` which I was able to set up and check locally on both platforms. Also tested on main.

I would really like to know if its possible to run this test in CI as [vasild suggeste
...

💬 pinheadmz commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#discussion_r2207665334)
The variable `local` in these lines is being used outside the `for` loop which defines it. If `localaddresses` is empty `[]` the error message itself will cause an error.

💬 fanquake commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-3073973824)
Put this on `30.0`.

💬 fanquake commented on pull request "doc: add alpine build instructions":
(https://github.com/bitcoin/bitcoin/pull/32559#discussion_r2207801898)
Did using the tracepoints work? Depends and a build will likely succeed, depends is just copying a header, and the CMake existence check is basic.

💬 waketraindev commented on pull request "rpc: add optional peer_ids param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#discussion_r2207805466)
I agree, the last push swapped to an unordered_set with an array argument

💬 rkrux commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2207804501)
This commented error should be removed now.

🤔 rkrux reviewed a pull request: "wallet, rpc: add v3 transaction creation and wallet support"
(https://github.com/bitcoin/bitcoin/pull/32896#pullrequestreview-3020910288)
Concept ACK b3817a822054fb2ea2964535b5a0ee55dd5d55ac
Thanks for picking this up, I will try to review it soon.

The TODOs seem to be done and can be removed from the PR description.

🤔 josibake reviewed a pull request: "Silent Payments: Receiving"
(https://github.com/bitcoin/bitcoin/pull/32966#pullrequestreview-3020599518)
Did a big overview with @Sjors , leaving the notes from our discussion as a review. In general, I think we should investigate using a custom type for the scan key since a lot of the changes seem to be hacking around the fact we represent the scan key as a private key, but then often need to use it as not a private key.

Still in the process of reviewing, but leaving the initial notes for now

💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207630652)
While reviewing the PR, it seems a lot of things could be simplified by if we just treated the scan key as something that _isn't_ a `CKey`, i.e., we introduce a completely new object for holding the scan key. I think that would allow us to drop this commit and simplify other places where we are trying to work around the fact that a scan key isnt really a private key.

I haven't thought this all the way through yet, but in my initial pass on the PR it seems like it should be possible.

💬 josibake commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2207667539)
Potentially another area we could simplify if we end up not treating a scan key as a `CKey`.