💬 fanquake commented on pull request "depends: build libnatpmp with CMake":
(https://github.com/bitcoin/bitcoin/pull/29708#discussion_r1578220994)
Possibly, but if we are going to do that, we could do it globally/all at once, or even set something at a higher level in depends. Not sure if we have to change this during these migrations.

💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1578242552)
You're right. I just tested here without wallet and it compiles. We don't have any verification for `ENABLE_WALLET` in `Makefile.test_fuzz.include` like in `Makefile.test.include`. I could include it.

💬 mzumsande commented on issue "Manually Banning Peers Results in Crash":
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2075482741)
> Hate to be the deliverer of bad news but I am also experiencing this issue on my Ubuntu installations after extensive testing.

I just tried in out under ubuntu, and banning both single and multiple peers worked fine.
What do you mean with "extensive testing"? If it happens always, just starting the program and banning a peer should be enough? Or does it happen only sometimes for you?

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