💬 achow101 commented on pull request "Descriptors: rule out unspendable miniscript descriptors":
(https://github.com/bitcoin/bitcoin/pull/27997#issuecomment-1639007336)
ACK c7db88af71b3204171f33399aa4f33b40a4f7cd9

🚀 achow101 merged a pull request: "Descriptors: rule out unspendable miniscript descriptors"
(https://github.com/bitcoin/bitcoin/pull/27997)

💬 jonatack commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1639025450)
Anything further needed here?

🤔 jonatack reviewed a pull request: "Descriptors: rule out unspendable miniscript descriptors"
(https://github.com/bitcoin/bitcoin/pull/27997#pullrequestreview-1533865920)
Post-merge ACK

💬 jonatack commented on pull request "subtree: update libsecp256k1 to latest master":
(https://github.com/bitcoin/bitcoin/pull/28093#issuecomment-1639117484)
Concept ACK

👍 ryanofsky approved a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1533749512)
This looks good, and thanks for all the clarifications. I'm just going over the commits another time in case I missed anything. Feel free to ignore all my suggestions, none are very important. Here's my progress so far:

- [X] fe86a7cd480b32463da900db764d2d11a2bea095 Explicitly track maximum block height stored in undo files (1/12)
- [X] 1cfc887d00c5d1d4281107e3b3ff4641c6c34631 Remove CChain dependency in node/blockstorage (2/12)
- [X] 471da5f6e74bac71aeffe2ebc5faff145a6cbcea Move block-arri
...

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1266007380)
In commit "Move block-arrival information / preciousblock counters to ChainstateManager" (471da5f6e74bac71aeffe2ebc5faff145a6cbcea)

The "reload of the block index" comment now seems out of date. And actually I don't see why this `ClearBlockIndexCandidates` call is necessary here. After the InitializeChainstate call above, the active chainstate should be newly created and `setBlockIndexCandidates` should already be empty. So I think it would be good to drop this `ClearBlockIndexCandidates` cal
...

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1266149966)
In commit "Tighten requirements for adding elements to setBlockIndexCandidates" (eb8ce95a2551fe521d00a5901ea773514395255b)

Is this comment in the commit message still accurate?

> Note that this introduces the new invariant that we can only have an assumeutxo
> snapshot where the snapshotted blockhash is in our block index. As a followup,
> unit tests that mock creating a snapshot ought to be updated to reflect this.

The only way I can see that it relates to code changes in this commit
...

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1266147442)
In commit "Tighten requirements for adding elements to setBlockIndexCandidates" (eb8ce95a2551fe521d00a5901ea773514395255b)

Note: test here is changed to add the real block 1, instead of a random block, so the block will not be ignored by the new `snapshot_base->GetAncestor(pindex->nHeight) == pindex` check in TryAddBlockIndexCandidate

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1266156727)
In commit "Tighten requirements for adding elements to setBlockIndexCandidates" (eb8ce95a2551fe521d00a5901ea773514395255b)

Seems like it might be useful to check `BOOST_CHECK_EQUAL(exp_tip, exp_tip2);` now to verify `ActivateBestChain` actually did arrive at the expected CBlockIndex*, not just the right block block hash.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1266153816)
In commit "Tighten requirements for adding elements to setBlockIndexCandidates" (eb8ce95a2551fe521d00a5901ea773514395255b)

Note: IIUC, it's necessary to load the genesis block here so the ActivateExistingSnapshot call below can be passed a known block hash of a block in `m_block_index`, not a random block.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1266161430)
In commit "Tighten requirements for adding elements to setBlockIndexCandidates" (b0b48b3e986319fe8cd5a80052099070279ce5c0)

Note: It seems like this LoadGenesis/SetBestBlock step was never necessary, so good to drop now. Dropping it maybe makes the test a little less realistic, but simplifies things and keeps the test more focused on verifying cache sizes.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1265982685)
re: https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1638374417, https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1237552792

> I wasn't sure if that was something to do in this PR or a future one?

I think it would be a good as a followup. Doing it wouldn't simplify this PR so it's probably good not to add it here.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1266117889)
re: https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1237511461

> Probably no need to use `ActiveChainstate()` for these (vs. `this->`) given the TODO above, but seems fine.

Marking resolved since later commit "Move CheckBlockIndex() from Chainstate to ChainstateManager" (f921887c1afdcd333cf71ca25e5efbf4991f806e) drops this.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1265971359)
re: https://github.com/bitcoin/bitcoin/pull/27746/commits/c8f4a8702851c3097ebd038a814d974dce4dcc78#r1251271941

Thanks, that makes sense, and thanks for linking to #5875.

To make this less confusing, I think "Try to process all requested blocks" code comment above could be updated to mention the active chain and better match the code.

Would suggest: "// Check all requested blocks that we do not already have for validity and save them to disk. Skip processing of unrequested blocks as an a
...

💬 ajtowns commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1266207614)
There's https://www.simple-is-better.org/json-rpc/transport_http.html (linked from the wikipedia page) which says 202 or 204; there's also https://www.simple-is-better.org/json-rpc/extension_transport.html by the same author but dated two months earlier, which suggests 200 or 204.

💬 MarcoFalke commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1639528790)
Concept ACK 20b49460b35268db59f7efcb02736b0e31191a74

Probably an edge case, but it seems useful to have this in case connections are opened at the same time for some reason.

💬 MarcoFalke commented on pull request "ci: document that -Wreturn-type has been fixed upstream (mingw-w64)":
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1639585127)
Looks like this triggers an OOM for some reason, when it shouldn't?

💬 S3RK commented on pull request "wallet: don't duplicate change output if already exist":
(https://github.com/bitcoin/bitcoin/pull/27601#discussion_r1266301780)
Let's document which part of `CoinSelectionParams` is modified and which is not.

💬 MarcoFalke commented on pull request "refactor: use Span for SipHash::Write":
(https://github.com/bitcoin/bitcoin/pull/28085#discussion_r1266314950)
> the error message "could not match 'Span' against 'vector'" still appears. (Or, did I misinterpret your second sentence and with "drop it" you meant to drop the template idea in general?).

Nah, clearly I don't understand C++. The overload *should* work, though, similar to the HexStr overloads:

```cpp
/**
* Convert a span of bytes to a lower-case hexadecimal string.
*/
std::string HexStr(const Span<const uint8_t> s);
inline std::string HexStr(const Span<const char> s) { return HexS
...