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

💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472986777)
I've experimented a bit with how far the range of inputs can be extended to cover extreme values (not because those extreme values are ones we care about in practice, but because they're more likely to expose bugs that do affect practical inputs):

```diff
--- a/src/wallet/test/fuzz/coinselection.cpp
+++ b/src/wallet/test/fuzz/coinselection.cpp
@@ -142,29 +142,25 @@ FUZZ_TARGET(coin_grinder_is_optimal)
FastRandomContext fast_random_context{ConsumeUInt256(fuzzed_data_provider)};

...

💬 mzumsande commented on pull request "test: fix intermittent failure in p2p_v2_earlykeyresponse":
(https://github.com/bitcoin/bitcoin/pull/29352#discussion_r1473024162)
thanks, renamed to `connection_opened`

💬 mzumsande commented on pull request "test: fix intermittent failure in p2p_v2_earlykeyresponse":
(https://github.com/bitcoin/bitcoin/pull/29352#issuecomment-1919339509)
Fixed both issues, thanks!

🤔 ryanofsky reviewed a pull request: "wallet: track mempool conflicts with wallet transactions"
(https://github.com/bitcoin/bitcoin/pull/27307#pullrequestreview-1853918541)
Code review 71059cf4267d80d2934b85a7046fc7b64900378b

I'm starting to review this, but I don't understand how it interacts with abandoned transactions. Is it still possible to abandon a transaction with conflicts with the mempool? If a transaction is already abandoned and we know it conflicts with the mempool does isMempoolConflicted still return false?

It seems to me like a more straightforward way to implement this feature would be to add a `bool mempool_conflicts` field to `TxStateInacti
...

💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1472930692)
In commit "wallet: use CWalletTx member functions to determine tx state" (86aaf81f81b48e82358db6700a6c9d46c5b5b0df)

Code comment here is just repeating the boolean expression and not explaining anything. I think the simplest explanation of this is "An output is considered spent by a spending transaction, unless the spending transaction is conflicted or abandoned." Could also add "But the check below is written to check for valid spending transactions instead of invalid ones so it isn't tied t
...

💬 ryanofsky commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1472955815)
In commit "wallet: track mempool conflicts" (a6ae5b23b1497ab6f4899db49348db623700a2d8)

I think it would be useful to add that mempool conflicted transactions are serialized as inactive transactions, so the distinction between TxStateMempoolConflicted and TxStateInactive only exists at runtime.

🤔 sr-gi reviewed a pull request: "test: fix intermittent failure in p2p_v2_earlykeyresponse"
(https://github.com/bitcoin/bitcoin/pull/29352#pullrequestreview-1854149947)
I wonder if we will not better off not having a simpler `initiate_v2_handshake` in this case and avoiding sync + async logic. Given we end up having to manually send raw messages and accessing `peer1` internal state anyway, I think it would be simpler to send everything manually:


```python
class TestEncryptedP2PState(EncryptedP2PState):
def __init__(self):
super().__init__(initiating=True, net='regtest')
self.magic_sent = False

def initiate_v2_handshake(self)
...