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

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