🚀 achow101 merged a pull request: "net: attempts to connect to all resolved addresses when connecting to a node"
(https://github.com/bitcoin/bitcoin/pull/28834)

🤔 stickies-v reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2023406729)
Approach ACK, code LGTM 66022315641934bda301d61f009dae9cb3b45da0 with some non-blocking suggestions.

> We could add back a safer, more limited form of operator= in the future if not having it causes friction.

I made my previous comment because I thought there are some instances of somewhat awkward `Update` usage in this PR. The majority of `util::Result` usage is/should probably not require `Update()` at all, because we're just returning either a success value or an error string. But, in t
...

💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580022711)
I think using `Update` here is confusing and makes the code less clear.

<details>
<summary>git diff on 6602231564</summary>

```diff
diff --git a/src/init.cpp b/src/init.cpp
index 8cb593754c..678bb2a39c 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -991,10 +991,8 @@ bool AppInitParameterInteraction(const ArgsManager& args)
InitWarning(strprintf(_("Reducing -maxconnections from %d to %d, because of system limitations."), nUserMaxConnections, nMaxConnections));

// *
...

💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580037125)
Alternative approach without `Update`:

<details>
<summary>git diff on 6602231564</summary>

```diff
diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp
index d0b06b726e..d9b4cdfdf2 100644
--- a/src/wallet/test/fuzz/notifications.cpp
+++ b/src/wallet/test/fuzz/notifications.cpp
@@ -106,13 +106,12 @@ struct FuzzedWallet {
CTxDestination GetDestination(FuzzedDataProvider& fuzzed_data_provider)
{
auto type{fuzzed_data_provider
...

💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580027359)
I don't think `Update` is appropriate here, would e.g. use:

<details>
<summary>git diff on 6602231564</summary>

```diff
diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp
index 2858494625..41ffb76762 100644
--- a/src/test/mempool_tests.cpp
+++ b/src/test/mempool_tests.cpp
@@ -202,9 +202,11 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest)
tx7.vout[1].scriptPubKey = CScript() << OP_11 << OP_EQUAL;
tx7.vout[1].nValue = 1 * COIN;

- auto ancestors_calculat
...

💬 stickies-v commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1580043133)
And also here, I think `Update` is confusing and can e.g. be simplified with:

<details>
<summary>git diff on 6602231564</summary>

```diff
diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp
index 640e63903a..963c0f838b 100644
--- a/src/wallet/test/spend_tests.cpp
+++ b/src/wallet/test/spend_tests.cpp
@@ -97,13 +97,11 @@ BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup)
// so that the recipient's amount is no longer equal
...

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