⚠️ 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).

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1578537557)
have you considered using `SoftSetBoolArg` to set `-walletbroadcast` to 0 if `-privatebroadcast` was chosen?
It's not the most user-friendly thing to introduce a new command arg that would only work if another arg was also changed from its default.

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1580115581)
It looks like we make sure in `net_processing` to not send any of theses messages, and here we check again. I haven't thought about this long, but can this log be triggered, or could this be an `Assume(false)` instead?

💬 davidgumberg commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1580221364)
On second thought it seems a bad idea to depend on the order in which tests are called for deciding whether or not to perform cleanup. I think that might be kind of a footgun for anyone modifying the test in the future

💬 davidgumberg commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2078316239)
ACK https://github.com/bitcoin/bitcoin/pull/29122/commits/be2c5510521081119f62ef3b29f76bb9097d0f34
Re-reviewed code, and attempted to break tests by making `ConsiderEviction` misbehave.

It is worth noting that these tests will not catch if outbound eviction logic gets triggered too early. e.g.:

```cpp
/** How long to wait for a peer to respond to a getheaders request */
static constexpr auto HEADERS_RESPONSE_TIME{0s};
// [...]
/** Timeout for (unprotected) outbound peers to sync to ou
...