💬 stickies-v commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1425373263)
A function called `AreInputsStandard` that returns a falsy value if inputs are standard is counterintuitive. Would suggest renaming to e.g. `HasNonStandardInput`.

<details>
<summary>git diff on 6ffb9569b7</summary>

```diff
diff --git a/src/bench/ccoins_caching.cpp b/src/bench/ccoins_caching.cpp
index c677fb1c16..9a31cecd7a 100644
--- a/src/bench/ccoins_caching.cpp
+++ b/src/bench/ccoins_caching.cpp
@@ -44,7 +44,7 @@ static void CCoinsCaching(benchmark::Bench& bench)
// Benchmar
...

💬 stickies-v commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1425375934)
nit: no need for `.has_value()`, idiomatic enough as is imo (here and in a bunch of other places)
```suggestion
BOOST_CHECK(!::AreInputsStandard(CTransaction(txTo), coins));
```

💬 stickies-v commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1425377178)
Since we've validated `has_value()` just above, can just derefence safely for conciseness:

```suggestion
return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, inputs_standardness_result->reason, inputs_standardness_result->debug);
```

💬 stickies-v commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1425431635)
nit: this is the reason for a single input
```suggestion
struct NonStandardInputReason {
```

💬 stickies-v commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1425494016)
nit: requires `#include <optional>` (in both files)

💬 stickies-v commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1425378615)
nit: more concise, easier to read (+ in other places)
```suggestion
assert(!AreInputsStandard(tx_1, coins));
```

💬 stickies-v commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1425491585)
This causes unnecessary input validation if `!m_require_standard`, would suggest changing to e.g.

<details>
<summary>git diff on 6ffb9569b7</summary>

```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 96204b2ab4..bf1c578384 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -822,9 +822,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
return false; // state filled in by CheckTxInputs
}

- const auto inputs_standardness_r
...

💬 stickies-v commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1425375326)
nit: No need for the intermediary var, here and in a bunch of other places

```suggestion
return NonStandardInputsReason{"bad-txns-input-script-nonstandard", strprintf("input %u", i)};
```

👋 hebasto's pull request is ready for review: "msvc: Fix `test\config.ini` content"
(https://github.com/bitcoin/bitcoin/pull/29075)

💬 hebasto commented on pull request "msvc: Fix `test\config.ini` content":
(https://github.com/bitcoin/bitcoin/pull/29075#issuecomment-1854135616)
Undrafted.

💬 mohamedawnallah commented on pull request "test: add TestNode wait_until helper":
(https://github.com/bitcoin/bitcoin/pull/29070#issuecomment-1854137832)
LGTM! Code Review ACK https://github.com/bitcoin/bitcoin/commit/bf0f7dbec6590a54ec890e7a2ca5d85427995334

💬 aureleoules commented on pull request "Replace non-standard CLZ builtins with c++20's bit_width":
(https://github.com/bitcoin/bitcoin/pull/29057#issuecomment-1854139729)
> I don't think the benchmarks will come. I guess the machine has been preempted by AWS? @aureleoules

I've re-ran the job. I actually found a way to re-run preempted jobs automatically!
A lot of benchmark jobs failed yesterday because of #29061 including this pull because I rebase all pulls on master before running jobs and I don't properly handle failures 😬

💬 hebasto commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1854145231)
> @hebasto any idea how to handle `D:\a\bitcoin\bitcoin\src\common\sv2_noise.h(92,13): error C3646: 'WriteMsg': unknown override specifier `?

> It's not easy being green

Happy to see CI is green :)

💬 mzumsande commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1425547546)
Yes, `ShouldReconnectV1` (which is called in `DisconnectNode()`) will result in the peer only being put on the list of reconnections if it's in `RecvState::KEY`. That is impossible if they ever sent us something, in which case `PerformReconnections()` will do nothing.

💬 mzumsande commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1854197471)
Also worth noting that while this PR deals with manual connections, we absolutely depend on the reconnection mechanism for automatic connections too:
The logic that we try with v1 based on service flags is just a courtesy but cannot be relied on, since an attacker could add the v2 service flag to arbitrary v1 nodes in most addrmans by spamming the network with the addr with an added v2 service flag, and then everyone thinks it's a v2 peer and depends on the reconnection mechanism when connecti
...

👍 BrandonOdiwuor approved a pull request: "bench: wallet, fix change position out of range error"
(https://github.com/bitcoin/bitcoin/pull/29065#pullrequestreview-1779977403)
ACK 37c75c58202f89b752500f76c872d7f8caf6bdb3

Looks good to me

👍 fanquake approved a pull request: "msvc: Fix `test\config.ini` content"
(https://github.com/bitcoin/bitcoin/pull/29075#pullrequestreview-1780042081)
ACK f76e59d02ea819928b45bd18d9c3a5b1aab36fe2

🚀 fanquake merged a pull request: "msvc: Fix `test\config.ini` content"
(https://github.com/bitcoin/bitcoin/pull/29075)

✅ fanquake closed an issue: "ci: MSVC failures"
(https://github.com/bitcoin/bitcoin/issues/29074)

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1425606237)
this only works if prioritization happens before mempool acceptance