💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1472836168)
Sounds good.

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472858661)
> Removing the check entirely for one of the options feels like a regression.

I don't think its a regression because this is the right behavior to check if the maximum fee rate we set is < minimum relay fee rate.


> but previously this code was checking that the user did not set a `maxtxfee` that was so low that the minrelay fee could never be met. Now you've removed that check, which means I can set a very low maxtxfee and not get a warning.

With the assumption of 1k vb size, transac
...

💬 epiccurious commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#issuecomment-1919164884)
utACK

📝 hernanmarino converted_to_draft a pull request: "assumeutxo, rpc: Add 'start' parameter to loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/28659)
Might fix https://github.com/bitcoin/bitcoin/issues/28620

This PR only modifies the behavior of loadtxoutset to require a mandatory "start" string as a first parameter.

This change will allow to implement "status", "abort" or similar future parameters in follow-ups, without breaking the current RPC protocol with incompatible changes (if this were to be merged in the current release cycle)

Also, it is implemented in a minimalist way, to make it easy to review.

💬 theStack commented on pull request "test: p2p: adhere to typical VERSION message protocol flow":
(https://github.com/bitcoin/bitcoin/pull/29353#issuecomment-1919238556)
Squashed the last two commits, as the second to last commit failed without the last bugfix commit ("test: p2p: process post-v2-handshake data immediately"); this should hopefully make the "test each commit" CI target happy.

📝 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`