💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1865922400)
Done, thanks

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1865922643)
enum class is safer than enum https://agrawalsuneet.github.io/blogs/enum-vs-enum-class-in-c++ and I didn't like the result of `using enum State`

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#issuecomment-2511657756)
Thanks @hodlinator, updated the PR:
* Removed redundant `inlines` from `struct CCoinsCacheEntry`
* Moved constants above `CoinEntry`and used const and brace init in more places in `coins_tests.cpp`
* Migrated changes inside the commits as soon as possible to minimize churn
* Added `AccessCoin` return value check in `CheckAccessCoin`
* Change `CheckAddCoin` param order to have cache values before the modified values in the row
* Fixed remaining `SetDirty`/`SetFresh` static reference in Fuzz
...

💬 maflcko commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2511693820)
... https://cirrus-ci.com/task/4678171796176896?logs=ci#L2624

💬 maflcko commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2511696412)
Is this test-only change with two reviews rfm?

This is the most common false positive right now.

💬 fanquake commented on pull request "[28.x] Backports & 28.1rc1":
(https://github.com/bitcoin/bitcoin/pull/31104#issuecomment-2511697266)
Thanks, fixed up the message.

💬 fanquake commented on pull request "build: Temporarily disable compiling `fuzz/utxo_snapshot.cpp` with MSVC":
(https://github.com/bitcoin/bitcoin/pull/31307#issuecomment-2511697621)
Added a commit for 28.x to #31104 to work around the same MSVC failure.

💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2511719781)
Concept ACK.

It would be nice if the asmap data blob didn't need to be converted to `std::vector<bool>` at runtime anymore, because that implies using memory twice (the blob itself, plus the decoded vector, despite containing effectively the same data). To improve upon that, I don't think it's hard to make the asmap interpreter code operate on a `std::span<const std::byte>` instead of a `std::vector<bool>`. Do you feel like looking into that, or do you want me to do so as a follow-up?

💬 instagibbs commented on pull request "test: Call generate RPCs through test framework only":
(https://github.com/bitcoin/bitcoin/pull/31403#issuecomment-2511736165)
any way you can see to prevent "regressions"?

💬 maflcko commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2511739493)
... https://cirrus-ci.com/task/6390830670282752?logs=ci#L4462

💬 maflcko commented on pull request "test: Call generate RPCs through test framework only":
(https://github.com/bitcoin/bitcoin/pull/31403#issuecomment-2511751001)
I don't see a trivial way to enforce this (even more) in Python, but given that there are only 9 lines that needed to be touched, it seems that the majority of code is fine, which is nice.

💬 darosior commented on pull request "Miner: never create a template which exploits the timewarp bug":
(https://github.com/bitcoin/bitcoin/pull/31376#issuecomment-2511789684)
> By normal circumstance you mean when nobody else on the network is attempting a timewarp attack? But what if there is one?

Sure, a timewarp attack is not normal circumstances. But even then this change would not be harmful. It would merely not be supporting attackers. Which as i argued in OP i see as a benefit.

> Let's say the soft fork is deployed in the future with MAX_TIMEWARP set to 300. But honest miner Alice is still running the v29 release with this PR it, using 600. Mean miner Bo
...

💬 ismaelsadeeq commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1866053557)
Happy to update when retouching.

💬 danielabrozzoni commented on issue "Unable to compile for test coverage on Nixos 24.05":
(https://github.com/bitcoin/bitcoin/issues/31087#issuecomment-2511853906)
Awesome, now it works! Thank you :)

💬 darosior commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#issuecomment-2511864012)
This has unfortunately gotten lower in my list of priorities. I'll close this PR for now to clarify its status. I hope to get back to this at some point. Anyone willing to work on this, or parts of this feel free to grab or/and reach out.

✅ darosior closed a pull request: "PoC: fuzz chainstate and block managers"
(https://github.com/bitcoin/bitcoin/pull/29158)

👍 willcl-ark approved a pull request: "[28.x] Backports & 28.1rc1"
(https://github.com/bitcoin/bitcoin/pull/31104#pullrequestreview-2473058165)
ACK 8fef83a0a03f884e0c5399b318eb55064b84b718

Agree that it's reasonable to apply the MSVC exclusion to all MSVC versions in this fixup.

<details>
<summary>system info</summary>

| k | v |
|-------------|-------|
| OS | Ubuntu 23.10 (mantic) |
| Arch | x86_64 |
| Kernel | 6.5.0-44-generic |
| System | Linux |
| CPU Model | 11th Gen Intel(R) Core(TM) i7-11700K @ 3.60GHz |
| CPU Cores | 16 |
| Memory | 62Gi |
| CC | Homebre
...

👋 brunoerg's pull request is ready for review: "fuzz: wallet: add target for spkm migration"
(https://github.com/bitcoin/bitcoin/pull/29694)

💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2511884253)
Force-pushed changing the fuzz target to address the `ApplyMigrationData` function. Now, after migrating the spk to descriptor, it creates some transactions and apply the migrated data.

Note that the CI failure is expected (see #31374):
```sh
[09:11:48.112] Run spkm_migration with args ['/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-max_total_time=60']INFO: Running with entropic power schedule (0xFF, 100).
[09:11:48.112] INFO: Seed: 2346481497
[09:11:48.1
...

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1866096570)
We are currently using the nullness of `m_config_path` to signal something different, initialized/not, although I think it should probably be more explicit. See https://github.com/bitcoin/bitcoin/pull/31212#issuecomment-2470510859, and this code:

https://github.com/bitcoin/bitcoin/blob/2ac863bdf974d7b4b4e8314876d02270400b6240/src/common/args.cpp#L761-L772

We could possibly do `optional<optional<path>> m_config_path` to flag both initialization and negation... but I'm not sure I want to go
...