💬 rkrux commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2207220067)
In fdba094a67906cc89918661e24c9c9a5e3f4cb0b "wallet: Load everything into DescSPKM on construction"

Some of them are now being used in `wallet.cpp` & `external_signer_scriptpubkeyman.h` and I suggested using couple in `walletdb.cpp` too. Maybe they can now go in a utility file like `walletutil.h`?

💬 rkrux commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2207281849)
In https://github.com/bitcoin/bitcoin/commit/93c3729ad5adc02f6d87ec8daa1a1952d7c41009 "wallet: Setup new autogenerated descriptors on construction"

This will make `storage` the first argument in all the 4 constructors and this consistency would be nice for readability imo.
```diff
- DescriptorScriptPubKeyMan(WalletBatch& batch, WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider);
+ DescriptorScriptPubKeyMan(WalletStorage& storage
...

💬 rkrux commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2207159782)
In https://github.com/bitcoin/bitcoin/commit/32dd0e35251973f9bdc2114e70c800624894d489 "wallet: Consolidate generation setup callers into one function"

This never seems to return `false`. Do we need to check for a `falsy` value while calling it like done above? Maybe not even return a bool? Just a thrown exception that is bubbled up.

💬 rkrux commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2207275810)
In 93c3729ad5adc02f6d87ec8daa1a1952d7c41009 "wallet: Setup new autogenerated descriptors on construction"

```diff
- //! Create an automatically generated DescriptorScriptPubKeyMan
+ //! Create an automatically generated DescriptorScriptPubKeyMan (i.e. during wallet or descriptor creation)
```

💬 rkrux commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2207233829)
In https://github.com/bitcoin/bitcoin/commit/fdba094a67906cc89918661e24c9c9a5e3f4cb0b "wallet: Load everything into DescSPKM on construction"

I feel these 2 can be kept inside `LegacyDataSPKM` itself, they are not used anywhere else, and I don't suppose they will be as well in the future.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2207333383)
> something like `doc/privacy`

Concept ACK. Out of the scope of this PR.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3073424315)
`6d697bba16...dbd76e68c3`: rebase due to conflicts

> runtime option for `sendrawtransaction`

@ismaelsadeeq, I thought about this, but decided to have a single global option to control the behavior given that the plan is to do private broadcast from other places as well. Otherwise it would be a messy situation where "my wallet is doing private broadcast but my RPC is not" or similar.

> what will a tor/i2p peer be able to do after fingerprinting that bunch of transactions are from it's pe
...

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2207478435)
you can resolve this

💬 pablomartin4btc commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2207512053)
nit: RPC is already in the title...
```suggestion
`upgradewallet` has been removed. It was unused and only applied to unsupported legacy wallets.
```

💬 instagibbs commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2207525287)
`max_iters` suffices, if you end up touching the PR

🚀 fanquake merged a pull request: "test: use notarized v28.2 binaries and fix macOS detection"
(https://github.com/bitcoin/bitcoin/pull/32922)

👍 instagibbs approved a pull request: "p2p: improve TxOrphanage denial of service bounds"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-3020524046)
ACK 50024620b909fc30b68a3715680e963f048482a5

Wish orphan traffic was higher for more live testing on mainnet. Will test anyways and report back if I see anything odd.

I'm not convinced that we really need EraseForBlock anymore, but I don't think it's a unique danger and it's nice that we get some real benchmarks for it in master.

💬 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.