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

💬 MarcoFalke commented on pull request "refactor: Make more transaction size variables `int32_t`":
(https://github.com/bitcoin/bitcoin/pull/28059#issuecomment-1639698787)
Are you still working on this?

💬 MarcoFalke commented on pull request "refactor: use Span for SipHash::Write":
(https://github.com/bitcoin/bitcoin/pull/28085#issuecomment-1639745003)
lgtm ACK 7d92b1430a6fd42c4438810640576830d0ff8d13

💬 MarcoFalke commented on issue "Spurious (?) valgrind failure for p2p_compactblocks.py":
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1639753496)
https://bugs.kde.org/show_bug.cgi?id=472329

💬 darosior commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1266463081)
I agree with Marco. My first reaction was hey we can't have our cake and eat it too, but in the case of the Miniscript targets we can: `miniscript_script` and `miniscript_string` could be left more generic by not discarding any coverage while `miniscript_smart` and `miniscript_stable` would.

🤔 darosior reviewed a pull request: "Add support for "partial" fuzzers that indicate usefulness"
(https://github.com/bitcoin/bitcoin/pull/27552#pullrequestreview-1534556964)
Concept ACK

💬 fanquake commented on pull request "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed"":
(https://github.com/bitcoin/bitcoin/pull/28077#issuecomment-1639826695)
> The I2P router may be in such a state

> Accept() fails because the I2P router sends something that is not Base64 on the socket:

It's not clear to me if this an issue in our code, or an issue with the router (or both), especially if everything "works fine" with one router, but not the other. Have these issues been reported upstream?

⚠️ fanquake opened an issue: "failure in wallet_resendwallettransactions.py --legacy-wallet"
(https://github.com/bitcoin/bitcoin/issues/28094)
https://cirrus-ci.com/task/5656433702731776?logs=ci#L7912 from #26288:
```bash
................................................................

194/268 - wallet_resendwallettransactions.py --legacy-wallet failed, Duration: 111 s
stdout:
2023-07-17T21:30:03.351000Z TestFramework (INFO): PRNG seed is: 4625691275301666838
2023-07-17T21:30:03.352000Z TestFramework (INFO): Initializing test directory /tmp/cirrus-ci-build/ci/scrat
...