💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1578283925)
```diff
diff --git a/src/Makefile.test_fuzz.include b/src/Makefile.test_fuzz.include
index 3f6cc30dc4..28f05c0968 100644
--- a/src/Makefile.test_fuzz.include
+++ b/src/Makefile.test_fuzz.include
@@ -13,8 +13,7 @@ TEST_FUZZ_H = \
test/fuzz/util.h \
test/fuzz/util/descriptor.h \
test/fuzz/util/mempool.h \
- test/fuzz/util/net.h \
- test/fuzz/util/wallet.h
+ test/fuzz/util/net.h

libtest_fuzz_a_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(BOOST_CPPFLAGS)

...

🤔 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