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

:lock: fanquake locked an issue: "Bitcoin with horcrux and powmem?"
(https://github.com/bitcoin/bitcoin/issues/29964)