📝 stratospher opened a pull request: "[test] make v2transport arg in addconnection mandatory and few cleanups"
(https://github.com/bitcoin/bitcoin/pull/29356)
- make `v2transport` argument in `addconnection` RPC mandatory. https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1470738750
- previously it was an optional arg with default `false` value.
- only place this RPC is used is in the [functional tests](https://github.com/bitcoin/bitcoin/blob/11b436a66af3ceaebb0f907878715f331516a0bc/test/functional/test_framework/test_node.py#L742) where we always pass the appropriate `v2transport` option to the RPC anyways.
- rename `v2_handshake()` to `
...

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1472938508)
done in #29356

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1472939091)
done in #29356

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1472939751)
done in #29356

💬 furszy commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1469679940)
This could be an `if (!m_pool.m_signals) continue;`, and be placed few lines above, at line 1217 (same for `AcceptSingleTransaction`).

👍 furszy approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1848819305)
Code review ACK 53642937. Only nits, nothing blocking. Nice work.

💬 furszy commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1472906800)
In 38d32ba2:

What about setting `callback_set=false` if `validation_signals` is null?

💬 furszy commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1472858901)
In 38d32ba2e9:

nit: I think that you could also remove the `<validationinterface.h>` include with this change.

💬 furszy commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1472869158)
nit:
```suggestion
node.validation_signals->RegisterSharedValidationInterface(outpoints_updater);
```

(same for the ones below)

💬 furszy commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1472896308)
In 38d32ba2:

This is not entirely accurate. `BlockChecked` and `NewPoWValidBlock` run synchronously.

💬 furszy commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1472920940)
nit:
```suggestion
node.validation_signals->RegisterSharedValidationInterface(txr);
```

Same for the others below.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1472944545)
didn't include this in #29356 because we'd need to call it in 2 places (before `initiate_v2_handshake`, `respond_v2_handshake`) and also calculate `garbage_len` there again.

💬 glozow commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472950555)
> Now you've removed that check, which means I can set a very low maxtxfee and not get a warning.

Perhaps just retain a version of the old check by enforcing that `maxtxfee` is set to at least `CAmount FLOOR_MAX_TXFEE{1000}` (a conversion from the default min relay feerate)?

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1472951364)
Leaving the check in with an `Assume` if we don't expect to hit it probably best, with a comment?

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1473018545)
Put the check in with an `Assume` and comment. Also renamed `ApplyV3Rules` to `SingleV3Checks`

🤔 sipa reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1853774778)
Concept ACK

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472849582)
Did you mean "then EXPLORE" here?

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472848173)
I think you should set the number of tries to be at least some multiple of the utxo_pool size, because (a) without it, you can't guarantee even considering every UTXO, and (b) the runtime of coin selection overall already contains a component proportional to the size of utxo_pool (simply the cost of constructing the pool in the first place) anyway. So I would suggest something like `const auto total_tries = max(TOTAL_TRIES_CG, 10 * utxo_pool.size());` or so.

It may be worth doing this as a fo
...

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472865513)
Coding style nit:

```c++
} else {
append_error(cg_result);
}
```

(single-line conditional only allowed if it's of the `if (condition) statement;` form)

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472864023)
This `result.SetSelectionsEvaluated(curr_try); break;` is repeated in both exit conditions. The `result.SetSelectionsEvaluated(curr_try);` can be moved outside of the loop I believe.