💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580051611)
> I haven't seen uses for a Replace method.

I agree. The use cases I was thinking of should just be addressed with list initialization, so I think not implementing the method until we actually need it is the way to go, thanks.

> Second, I think the name Merge would be misleading ... only different fields are combined

That's a good point, I agree `Update` > `Merge`

> it's less obvious what a.Combine(b) would be supposed to do

I don't think I agree, but I do agree there's no clear w
...

💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#issuecomment-2078096227)
Moving this to draft given it is dependant on https://github.com/bitcoin/bitcoin/pull/28016

📝 sr-gi converted_to_draft a pull request: "net: Favor peers from addrman over fetching seednodes"
(https://github.com/bitcoin/bitcoin/pull/29605)
This is a follow-up of #28016 motivated by https://github.com/bitcoin/bitcoin/pull/28016#pullrequestreview-1913140932 and https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-1984448937.

The current behavior of seednode fetching is pretty eager: we do it as the first step under `ThreadOpenNetworkConnections` even if some peers may be queryable from our addrman. This poses two potential issues:

- First, if permanently set (e.g. running with seednode in a config file) we'd be signaling
...

💬 TheCharlatan commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2078100935)
Thanks for taking another look at @ryanofsky!

Updated 30a1024b63105a97d04149e83ae2d8cf0830cf69 -> d447bdcfb0e38353940e4a7fc89d09482d8d39c3 ([mempoolArgs_5](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_5) -> [mempoolArgs_6](https://github.com/TheCharlatan/bitcoin/tree/mempoolArgs_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/mempoolArgs_5..mempoolArgs_6))

* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-2022700917
...

⚠️ lucasnuic opened an issue: "Bitcoin with horcrux and powman?"
(https://github.com/bitcoin/bitcoin/issues/29964)
### Please describe the feature you'd like to see added.

Currently Bitcoin does not have an algorithm that I call MFAW(Multi-Factor Authentication Wallets). But most can implement this by adopting different types of wallets, and there is even bitcoin documentation about this. For example, you could say that there is security in Bitcoin through "wallets" or MFAW(Multi-Factor Authentication Wallets)". To have or obtain Multi-Factor Authentication Wallets simply have x amount of bitcoin for each w
...

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#issuecomment-2078149594)
Thanks @stickies-v, I agree with your suggestions to avoid updating existing `Result` objects where possible and will apply them.

But I wonder if there are is anything else I can do to help make the interface more intuitive though. I'm not exactly clear on why it is not intuitive currently. A few places you mentioned "Update here is confusing" but I don't understand why it is confusing. Saying `result.Update(success_value)` is just meant to update the result object with a success value while
...

✅ lucasnuic closed an issue: "Bitcoin with horcrux and powmem?"
(https://github.com/bitcoin/bitcoin/issues/29964)

💬 theuni commented on pull request "depends: Do not consider `CC` environment variable for detecting system":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2078169426)
Surely this isn't the only place env vars like this would cause issues? Feels like a cat-and-mouse game.

🤔 ryanofsky reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2023574807)
Updated 66022315641934bda301d61f009dae9cb3b45da0 -> 4fb08c28ba0f14782fdab4ac54387aae0aba82ed ([`pr/saferesult.3`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.3) -> [`pr/saferesult.4`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/saferesult.3..pr/saferesult.4)) with suggested changes using new result objects to avoid updating existing ones where possible

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580124463)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580022711

Thanks, applied patch

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580124595)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580027359

Thanks, applied patch

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580125044)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580043133

Thanks, applied patch

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580124847)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580037125

Thanks, applied patch

💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#issuecomment-2078213252)
Sorry, I've been doing a poor job explaining myself. I am fully onboard with how `Update` works. When I used the word "confusion", I was talking about _usage_, not interface*. Prior to this push, `Update()` was used in a couple of places where we know or expect the `Result` to be empty, and in those case I think we should just be explicit about that and use list initialization. When I see `result.Update()` being called, I have to look up which values are in `result` already and ensure that's co
...

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1579854025)
You probably meant `onion_arg != "0"`

🤔 mzumsande reviewed a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-2020968807)
Did another round of review.

nit: `local tx relay` -> `private broadcast` in last commit message

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1580149990)
This class is getting pretty large in the end, maybe put it into its own module (similar to `TxReconciliationTracker` or `TxRequestTracker`)

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1579852847)
I think this requires an update to `tor.md`, which currently says:
`-onion=ip:port Set the proxy server to use for Tor onion services. You do not need to set this if it's the same as -proxy.`
After this PR, specifiying it explicitly is necessary if you want to do private broadcast to clearnet peers.

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1580200865)
I don't think is needed, `MEMPOOL` is never tested.

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1580205599)
sendrawtransaction doc should be updated (it already mentions privacy issues).