📝 maflcko opened a pull request: "doc: Assert that assumed-valid blocks are not fully valid in CheckBlockIndex()"
(https://github.com/bitcoin/bitcoin/pull/29355)
It does not make sense for a block to be fully-valid and assumed-valid at the same time.

Check it in `CheckBlockIndex()` and fix the tests that violate this assumption.

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472820140)
Yes but I think should come in a follow-up after cleaning up your suggestion here
https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1472790897

I have a local branch that does this on top of this.

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1472822671)
@glozow's analysis looks right to me. I don't think it matters too much way we go -- either we should update the code comments to explain that correctness relies on invoking ApplyV3Rules and PV3C on package transactions, or we can just leave the duplicate check in here so that PV3C stands on its own for package validation. I'd probably vote for just updating the comment but I don't feel strongly either way.

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