👍 TheCharlatan approved a pull request: "ci: remove `python3-setuptools` from macOS build deps"
(https://github.com/bitcoin/bitcoin/pull/28932#pullrequestreview-1747457083)
ACK 0ffcc5b680b92cf921fc11afb05e4a3607572d41

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1404092298)
Improved the documentation around it a bit.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1825334360)
Addressed the comments, mostly refactoring. Some conversations pending above. The code is good for review.

💬 maflcko commented on issue "wallet RPC to double-check the calculated balance":
(https://github.com/bitcoin/bitcoin/issues/28898#issuecomment-1825338315)
> An alternative/parallel course of action could be making `rescanblockchain` much faster. Removing the need for additional RPC calls to identify any missing tx.

I don't think this is an alternative, because the rescan won't find the txs if the birthdate is wrong, see also https://github.com/bitcoin/bitcoin/issues/28897? Moreover, on hardware without parallel cores, building the block filter index is not going to be sped up. Finally, on wallets with a sufficient number of keys, the blockfilte
...

💬 naumenkogs commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1404107883)
good point.

💬 naumenkogs commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1404108378)
well, actually....you still can track it with a map<addr, bool>, no?

👍 dergoegge approved a pull request: "fuzz: Limit fuzz buffer size in script_flags target"
(https://github.com/bitcoin/bitcoin/pull/28931#pullrequestreview-1747698896)
ACK faf1fb207fb6e9a12c864074f8c40d5922d93ff4

💬 dergoegge commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-1825460284)
Concept ACK

💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1825462831)
> Our service rests on the first seen rule. Based on our experience a trx with confirmed inputs, reasonable fee, not seen conflicting trx for  5-7 seconds - is reliable. Also the trxs do not happen in a vacuum but rather in commercial exchanges with expected characteristics.

Again, you still have refused to answer my question: Have you actually tested full-RBF double spends against your service?

💬 hebasto commented on pull request "ci: Avoid toolset ambiguity that MSVC can't handle":
(https://github.com/bitcoin/bitcoin/pull/28905#issuecomment-1825472700)
> Do we know why this broke again? I guess this is something we'll just have to maintain forever?

I've done a bit more research.

In GHA Windows images, it is an accepted practice to have different versions of MSVC toolsets being installed side-by-side.

For example, the current image 20231115 has the following toolsets installed:
- 14.29.16.11
- 14.37.17.7
- 14.38.17.8

The [`ilammy/msvc-dev-cmd`](https://github.com/ilammy/msvc-dev-cmd) action chooses the latest compiler toolset ver
...

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1404207499)
Good point indeed. To execute the steps build/encrypt/send-message atomically, I think introducing a new lock and acquiring it in the `send_message` method should do the trick? I.e.
```diff
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index a70682a768..59707471a6 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -164,6 +164,7 @@ class P2PConnection(asyncio.Protocol):
# The underlying tra
...

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1404234919)
+1, good catch! when i tried this, just running `self.test_resource_exhaustion()` in `p2p_invalid messages.py` took too much time.
1. in v1, without locks - 0m22.265s
2. in v2, without locks - won't work
3. in v1, with locks - 0m22.101s
4. in v2, with locks - 8m45.916s

💬 maflcko commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1404246109)
`*std::get_if` may be UB? In any case, it would be good to submit fuzz inputs that cover everything here.

💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-1825561375)
Rebased and addressed comments

- https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1155488768: @martinus I took parts from your suggestion and added you as a co-author for the first commit. Thanks for the macro-magic. There's now only the `TRACEPOINT()` macro that works for tracepoints with 0 to 12 args.
- https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1140555021: I clarified unit tests and added a new test for a manual `TRACEPOINT_ACTIVE()` macro.
- https://github.com/bi
...

📝 maflcko opened a pull request: "fuzz: Faster wallet_notifications target"
(https://github.com/bitcoin/bitcoin/pull/28933)
Avoid read/write from storage to speed the target up.

👍 willcl-ark approved a pull request: "Fee Estimator updates from Validation Interface/CScheduler thread"
(https://github.com/bitcoin/bitcoin/pull/28368#pullrequestreview-1746716540)
ACK 91504cbe0de2b74ef1aa2709761aaf0597ec66a2

Glad to see the fee estimator being decoupled in this way and keen to see more development of additional estimators in the future e.g. as per #27995

I gave the code a review and it seems well thought-out to me at this stage; left a few nits which can be addressed in the case that you re-touch things.

💬 willcl-ark commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1403539773)
nit if you retouch bfcd401368fc0dc43827a8969a37b7e038d5ca79:

```suggestion
* Provides the block that was disconnected.
```

💬 willcl-ark commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1404174636)
micro nit, if you end up touching 714523918ba2b853fc69bee6b04a33ba0c828bf5 again:

```suggestion
// Drop transactions we were still watching, record fee estimations and unregister
```

💬 willcl-ark commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1404159173)
In dff5ad3b9944cbb56126ba37a8da180d1327ba39

Worth adding a doxygen comment here? Even something simple like:

```cpp
/**
* Holds information about new transactions added to the mempool.
* In addition to TransactionInfo includes limit bypass, package, chain and parent info.
*/
```

💬 willcl-ark commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1404305799)
In 714523918ba2b853fc69bee6b04a33ba0c828bf5

Would we want to use `fuzzed_data_provider.ConsumeBool()` for `m_submitted_in_package`?

💬 willcl-ark commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1404169179)
In dff5ad3b9944cbb56126ba37a8da180d1327ba39

You've called this `m_from_disconnected_block`, but described it as whether the tx was added without enforcing mempool fee limits. This seems incorrect or confusing.

`SubmitPackage()` is calling this using `args.m_bypass_limits` so i think the comment is correct, and the variable should be renamed?