👍 vasild approved a pull request: "build: Remove Autotools-based build system"
(https://github.com/bitcoin/bitcoin/pull/30664#pullrequestreview-2266075367)
ACK debeb98fe3822962768127e5b5d312926f123211

I already reviewed this at https://github.com/hebasto/bitcoin/pull/166. Only minor changes since that + doc adjustment.

💬 hebasto commented on pull request "build: remove old MSVC build system":
(https://github.com/bitcoin/bitcoin/pull/30731#discussion_r1734563913)
I'm fine with any approach as well.

💬 hebasto commented on pull request "build: remove old MSVC build system":
(https://github.com/bitcoin/bitcoin/pull/30731#discussion_r1734567733)
> But build directories that aren't called `build_msvc` can still linger aorund in the same way. i think it's strange to keep this around but ok.

Other `build*` directories are created by the user explicitly, and the user is aware of them. However, the presence of the `build_msvc` directory might be unxepected.

💬 vasild commented on pull request "build: Remove Autotools-based build system":
(https://github.com/bitcoin/bitcoin/pull/30664#issuecomment-2315161976)
from https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2314866592:

> I am not going to block the autotools removal

then, here:

> NACK

Eh?

💬 laanwj commented on pull request "build: remove old MSVC build system":
(https://github.com/bitcoin/bitcoin/pull/30731#issuecomment-2315164107)
ACK 04fb085f6bddc90e806deefd90c72104763f055a

👍 TheCharlatan approved a pull request: "build: remove old MSVC build system"
(https://github.com/bitcoin/bitcoin/pull/30731#pullrequestreview-2266095576)
ACK 04fb085f6bddc90e806deefd90c72104763f055a

Good riddance.

👍 danielabrozzoni approved a pull request: "test: fix `TestShell` initialization (late follow-up for #30463)"
(https://github.com/bitcoin/bitcoin/pull/30714#pullrequestreview-2266107493)
tACK bd7ce05f9d9d21903163a2bd9dd2df3ed3990c3e

I agree that the approach feels a bit hacky, but I can't see any other simple way to achieve the same result. This seems good enough, and the fix is pretty isolated :)

💬 maflcko commented on pull request "build: Remove Autotools-based build system":
(https://github.com/bitcoin/bitcoin/pull/30664#issuecomment-2315185211)
> Eh?

Maintainers are free to ignore a review, but as per my previous comment, I think rushing this in without further and proper review will just lead to more follow-up pulls dealing with the stuff that was forgotten to be removed. Also, it is going to make parity testing harder while doing cmake follow-ups.

I just don't see why this needs to be rushed, when there is no downside or even risk in waiting a few days at a minimum.

If I am missing a downside, it would be good to mention it.
...

👍 vasild approved a pull request: "build: remove old MSVC build system"
(https://github.com/bitcoin/bitcoin/pull/30731#pullrequestreview-2266113698)
ACK 04fb085f6b, except:

```
1: ad12ccc5d1 ! 1: 04fb085f6b build: Remove legacy MSVC build system
@@
## Metadata ##
-Author: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
+Author: fanquake <fanquake@gmail.com>
```

I already reviewed this at https://github.com/hebasto/bitcoin/pull/166

🚀 fanquake merged a pull request: "build: remove old MSVC build system"
(https://github.com/bitcoin/bitcoin/pull/30731)

🤔 ismaelsadeeq reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2266000970)
Approach ACK

💬 ismaelsadeeq commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1734517492)
At first I was wondering if their is any benefit of not declaring the implementation class private to `TxDownloadManager` as recommended in https://en.cppreference.com/w/cpp/language/pimpl.

But then after the first pass of this PR, I think the reason is to make `TxDownloadManagerImpl` visible to the entire namespace in other to allow for testing `TxDownloadManagerImpl` specific behaviors in fuzzing.

💬 ismaelsadeeq commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1734554084)
```suggestion
AssertLockHeld(m_tx_download_mutex); // For m_txdownloadman
```

💬 ismaelsadeeq commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1734535670)
In 58fb38c182906645161af04e84becc824dee2ba5 " [refactor] add TxDownloadManager wrapping TxOrphanage, TxRequestTracker, and bloom filters"


I think the file name should be txdownloadman.cpp ?
`TxDownloadManager` is the public interface.

💬 ismaelsadeeq commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1734565020)
The update was effected in the second commit

🤔 stickies-v reviewed a pull request: "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface"
(https://github.com/bitcoin/bitcoin/pull/30697#pullrequestreview-2265996536)
post-merge ACK 1b41d45d462d856a9d0b44ae0039bbb2cd78407c

I think some follow-up improvements should be made as per my review comments, but this PR addresses the race condition well.

> Wallet code originally did used ArgsManager directly until https://github.com/bitcoin/bitcoin/pull/22217, which changed it to use an interface class so wallet and node code could run in different processes and both could change settings without getting out of sync or corrupting the file.

Thanks for the cont
...

💬 stickies-v commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1734595665)
Note: behaviour already existed prior to this PR, but this line (and thus also `overwriteRwSetting`) can throw if `name` is not an existing key, and would benefit from being documented in both function's docstring I think.

💬 stickies-v commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1734540462)
Moving an lvalue ref here isn't safe, I think? Not an issue with the current callsites, but function signature should either take an rvalue ref or make a copy here. E.g. impl with rvalue ref (which is sufficient for now, but may require an overload in future:

<details>
<summary>git diff on 1b41d45d46</summary>

```diff
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index be596b1765..20a801fa94 100644
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -16,6 +16,
...

💬 stickies-v commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1734601725)
I think this should be simplified to:

<details>
<summary>git diff on 1b41d45d46</summary>

```diff
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 54b986c926..b1699bc7ae 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -819,10 +819,8 @@ public:
{
std::optional<interfaces::SettingsAction> action;
args().LockSettings([&](common::Settings& settings) {
- auto* ptr_value = common::FindKey(settings.rw_settings,
...

💬 stickies-v commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1734616135)
nit: there's no need for a `WalletContext` here:

<details>
<summary>git diff on 1b41d45d46</summary>

```diff
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 12d5a3b3eb..7597765b24 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -333,12 +333,11 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
// concurrently, ensuring no race conditions occur during either process.
BOOST_FIXTURE_TEST_C
...