🤔 achow101 reviewed a pull request: "Fix waste calculation in SelectionResult"
(https://github.com/bitcoin/bitcoin/pull/28366#pullrequestreview-2020564522)
ACK e95b9159380f2de7f9a6e7a202cc171ad285ee6c

💬 achow101 commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1578299095)
In 44db79b0c48d6b1a26dba6ab01c2a296d610c06b "Refactor ComputeAndSetWaste()"

nit: Unnecessary parentheses, also usually the check of the value is after the function. Could also use brace initialization.

```suggestion
bool makes_change{GetChange(min_viable_change, change_fee) != 0};
```

✅ achow101 closed a pull request: "Add Gutter Guard Selector"
(https://github.com/bitcoin/bitcoin/pull/28977)

💬 achow101 commented on pull request "Add Gutter Guard Selector":
(https://github.com/bitcoin/bitcoin/pull/28977#issuecomment-2075532537)
Closing as this is going to be superseded by Sand Compactor

👍 ryanofsky approved a pull request: "kernel: De-globalize fReindex"
(https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2020582214)
Code review ACK 9b2c9c2fce32fe858d1361e863c72108a384a5c8. This looks good in its current form. I just had one suggestion below (not important) and some questions about possible minor pre-existing bugs.

re: https://github.com/bitcoin/bitcoin/pull/29817#issuecomment-2067781043

> I thought abour this initially as well, but the way I read the code, this is actually a change in behaviour [...]

Thanks for the explanation, that makes sense.

💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1578309261)
In commit "kernel: De-globalize fReindex" (9b2c9c2fce32fe858d1361e863c72108a384a5c8)

I think it would be less fragile to assign `blockman_opts.reindex` instead of `chainman.m_blockman.m_reindex`.

Assigning `blockman_opts.reindex` should ensure and make it more obvious that this is only affected by the value of the `-reindex` setting, not the value of a previous setting saved with `WriteReindexing`.

💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1578320266)
In commit "kernel: De-globalize fReindex" (9b2c9c2fce32fe858d1361e863c72108a384a5c8)

Note: I initially thought there was a minor bug in existing code (independent of this PR) on line 1605 above which checks `if (!options.reindex)` instead of `if (!chainman.m_blockman.m_reindexing)`. It seemed wrong because it could trigger an error suggesting using reindex flag even if reindexing was already happening from a previous request. This behavior is surprising, but on second thought, could actually
...

💬 ryanofsky commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1578327024)
In commit "kernel: De-globalize fReindex" (9b2c9c2fce32fe858d1361e863c72108a384a5c8)

It seems like there is a prexisting bug here, and the `f_wipe` argument passed here should be `blockman_opts.reindex` instead of `chainman.m_blockman.m_reindexing` or `fReindex`. Otherwise if the node is restarted before reindexing completes the TxIndex will be wiped a second time?

Same for BlockFilterIndex and CoinStatsIndex below

🤔 mzumsande reviewed a pull request: "net: Make AddrFetch connections to fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/26114#pullrequestreview-2020650571)
Thanks for the reviews and sorry for not following up earlier!
I hope I have now addressed all comments.

I also rebased wrt master - strangely github doesn't seem to detect some merge conflicts in this PR, locally I had conflicts with both #28170 and #29850 - different algorithms maybe?

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1578349512)
> I agree. I've tested it following `contrib/seeds/README.md` to generate the seeds and the worst scenario was connecting with 5 of the 10.

Yes, I see similar results. If older versions are used, it could become a bit worse though.

> nit: the log might use a variable 2, so that it's defined in a single place.

I have now used a variable for the 2 minutes (sorry, I missed this comment before).

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1578356506)
I have reordered the sentence now so that it is clearer now hopefully!

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1578357274)
updated the doc!

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1578358302)
Good catch! removed the mention of ThreadOpenConnections in the following commit (to keep it out of the already large move commit)

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1578355015)
added the 10

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1578355641)
done, thanks!

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1578354870)
I agree that could be good! Though I think it would add more complexity to this pull, so I would prefer not to do it here - could be a good follow-up though.

Just noting that we do add all fixed seeds to addrman though, so with the connection logic from #27213 we'd eventually still make a connection to the network.

📝 MarnixCroes opened a pull request: "guiconstants: update ORG_DOMAIN to bitcoincore.org"
(https://github.com/bitcoin-core/gui/pull/818)
if desired, can also update `ORG_NAME` to _Bitcoin Core_
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
signific
...

💬 achow101 commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#issuecomment-2075656821)
ACK modulo existing comments.

🤔 ryanofsky reviewed a pull request: "Disable util::Result copying and assignment"
(https://github.com/bitcoin/bitcoin/pull/29906#pullrequestreview-2020422224)
Updated c45cb13b9e4f6eae50ab6dc42acf471921980591 -> 66022315641934bda301d61f009dae9cb3b45da0 ([`pr/saferesult.2`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.2) -> [`pr/saferesult.3`](https://github.com/ryanofsky/bitcoin/commits/pr/saferesult.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/saferesult.2..pr/saferesult.3)) adding change to actually disable the copy constructor

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1578217883)
re: [#29906 (comment)](https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1572632242)

> nit: Any reason not to just wrap the args in `std::move`? Seems like the more intuitive approach, and probably slightly more performant?

Actually this turns out not to work because `std::initializer_list` list [does not work](https://old.reddit.com/r/cpp/comments/7a1o7f/why_does_stdinitializer_list_prevents_using/) with move-only types. So unfortunately vector initialization needs to be a little
...