💬 maflcko commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1578211737)
Actually I don't understand why the last push compiles. CI says:

* https://cirrus-ci.com/task/6011306526900224?logs=ci#L2291 ` CXX test/fuzz/util/libtest_fuzz_a-wallet.o`
* but the wallet is disabled: ` with wallet = no` https://cirrus-ci.com/task/6011306526900224?logs=ci#L1385

Shouldn't this result in a link or even compile error?

👍 vasild approved a pull request: "doc: Bash is needed in gen_id and is not installed on FreeBSD by default"
(https://github.com/bitcoin/bitcoin/pull/29953#pullrequestreview-2020413434)
ACK 9381052194a78024b3994cc6ad906858c477b88f

💬 m3dwards commented on pull request "depends: build libnatpmp with CMake":
(https://github.com/bitcoin/bitcoin/pull/29708#discussion_r1578213646)
Perhaps this is what we want but I thought it worth mentioning that there is a difference on my Mac between this and having cmake call make with: `cmake --build . --target natpmp`.

$(MAKE) from autotools for me calls GNU MAKE 3.81 and cmake delegates to gmake being GNU MAKE 4.4.1.

Should we call cmake instead and allow it to decide the best make to delegate to?

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