💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1640702774)
Are you still working on this?

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1640704021)
> Are you still working on this?

Absolutely, thanks. Had a few other things to catch up on. This will be updated before the end of the week

💬 MarcoFalke commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1640710663)
We already use credits for some Linux tasks, so nothing will change for them. The others (or all) can be moved to persistent workers easily, I suspect.

Edited OP to mention GitHub Actions CI as alternative. Last time we looked it required write access, but GitHub already has full write access, so I am not sure if this is still a concern or if it even applies?

💬 darosior commented on pull request "Detect and ignore transactions that were CPFP'd in the fee estimator":
(https://github.com/bitcoin/bitcoin/pull/25380#issuecomment-1640745025)
Rebased.

📝 sipa opened a pull request: "crypto: ChaCha20 `Span<std::byte>` modernization & follow-ups"
(https://github.com/bitcoin/bitcoin/pull/28100)
This modernizes the ChaCha20 and ChaCha20Aligned interfaces to be `Span<std::byte>` based, and other improvements.

* Removes our OpenSSH-inspired variant of the ChaCha20Poly1305 AEAD (which was added in anticipation of BIP324 using it, but the spec was later changed to using the RFC8439 one unmodified). #28008 replaces the code anyway, so it is pointless to apply the refectoring done in this PR to that old code.
* Modifies all functions and constructors of `ChaCha20` and `ChaCha20Aligned` to
...

💬 sipa commented on pull request "Reduce wasted pseudorandom bytes in ChaCha20 + various improvements":
(https://github.com/bitcoin/bitcoin/pull/26153#discussion_r1267164225)
Done in #28100.

💬 sipa commented on pull request "Reduce wasted pseudorandom bytes in ChaCha20 + various improvements":
(https://github.com/bitcoin/bitcoin/pull/26153#discussion_r1267164605)
Done in #28100.

💬 sipa commented on pull request "Reduce wasted pseudorandom bytes in ChaCha20 + various improvements":
(https://github.com/bitcoin/bitcoin/pull/26153#discussion_r1267164850)
Done in #28100.

💬 sipa commented on pull request "Reduce wasted pseudorandom bytes in ChaCha20 + various improvements":
(https://github.com/bitcoin/bitcoin/pull/26153#discussion_r1267165531)
I had forgotten about this PR comment, but this was effectively done in #27985.

💬 sipa commented on pull request "Reduce wasted pseudorandom bytes in ChaCha20 + various improvements":
(https://github.com/bitcoin/bitcoin/pull/26153#discussion_r1267165897)
Done in #28100.

💬 jonatack commented on pull request "crypto: ChaCha20 `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267196895)
In commit 9e1199c7ce5

```
random.cpp:668:44: error: non-type template argument is not a constant expression
static constexpr std::array<std::byte, rng.KEYLEN> ZERO{};
^~~~~~~~~~
random.cpp:668:44: note: implicit use of 'this' pointer is only allowed within the evaluation of a call to a 'constexpr' member function
1 error generated.
```

```
$ clang --version
Homebrew clang version 16.0.6
Target: arm64-apple-darwin22.5.0
Thread model:
...

💬 sipa commented on pull request "crypto: ChaCha20 `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267203493)
Strange that GCC doesn't complain about this. Fixed.

👍 ryanofsky approved a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1535703245)
Code review ACK 137762f34a845e491b80f9cea07efc4427cb38bf. Left some more suggestions, but this looks good in its current form.

Just so I don't forget, I want to note that that it might make sense to move `IsInitialBlockDownload` and `NotifyHeaderTip` functions from the `Chainstate` class to `ChainstateManager` as a followups to this PR (https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246868905, https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1237552792)

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1267184143)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (f41cef2ba1e1f033356691e520e39d9d75d5eb4f)

Note: the reason why all entries were added previously was because `pindex->nHeight < first_assumed_valid_height` check was always true, because `first_assumed_valid_height` was `std::numeric_limits<int>::max()`

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1267180167)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (f41cef2ba1e1f033356691e520e39d9d75d5eb4f)

It would be good to remove the reliesOnAssumedValid() function since it is no longer used after this.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1267187714)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (f41cef2ba1e1f033356691e520e39d9d75d5eb4f)

HAVE_DATA flag doesn't actualy seem to be removed here. Should it be? And since the test is already passing withtout this would the reason just removing HAVE_DATA flag be to satisfy CheckBlockIndex checks?

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1267213310)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (f41cef2ba1e1f033356691e520e39d9d75d5eb4f)

I'm actually confused about how this is working because the HAVE_DATA flag doesn't seem to be removed above.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1267209493)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (f41cef2ba1e1f033356691e520e39d9d75d5eb4f)

Command at the top of the test saying it checks cs2 "contains all blocks, even those assumed-valid" is out of date. Should be "contains all blocks except those assumed-valid, because they don't have data"

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1267204896)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (f41cef2ba1e1f033356691e520e39d9d75d5eb4f)

This is true, but it seems like it might make the test less confusing and more realistic if we just added a ` cs2.m_chain.SetTip(* assumed_base)` call below the `cs1.m_chain.SetTip(*validated_tip);` call. I think if this was done the cs2.setBlockIndexCandidates would just contain blocks after the snapshot.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1267227843)
In commit "Documentation improvements for assumeutxo" (bef5acee93e128857d20137ea0c83f281a780211)

"below the snapshot height" should be "at the snapshot height and below"