💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1645856227)
It's not really a readability suggestion, I just prefer having `requested_nums_elems` be the integer type for which we're doing the bounds checking. But it's just a nit so happy to leave as is.

💬 maflcko commented on pull request "fuzz: Improve stability for txorphan and mini_miner harnesses":
(https://github.com/bitcoin/bitcoin/pull/30306#discussion_r1645859625)
Ah sorry. I was confused by `std::vector<MockEntryMap::iterator> m_entries;`, but the iterator isn't into the vector, but into the map itself. Discussion can be closed.

💬 maflcko commented on pull request "fuzz: Improve stability for txorphan and mini_miner harnesses":
(https://github.com/bitcoin/bitcoin/pull/30306#discussion_r1645863239)
> (the first column indicates afll+'s stability metric, these links will stop working at some point)

Nice. If you don't mind, could share the steps to create the stats.txt file?

🤔 glozow reviewed a pull request: "fuzz: have package_rbf always make small txns"
(https://github.com/bitcoin/bitcoin/pull/30300#pullrequestreview-2127790734)
ACK 4ccb3d6d0d576d32da8a1b9c6e70962cbd0f19fe

Agree value of `GetTxSize()` is chosen by `ConsumeTxMemPoolEntry` and conflicts aren't calculated using outpoints, so there's no need to make mempool transactions through `ConsumeDeserializable<CMutableTransaction>`.

seed 95a3b1b42d43ac7190f24ff116b2cd6515e7a566 didn't give me an oom but took 1981ms / now takes 71ms.

💬 dergoegge commented on pull request "fuzz: Improve stability for txorphan and mini_miner harnesses":
(https://github.com/bitcoin/bitcoin/pull/30306#discussion_r1645874231)
Maybe but this sounds like more effort than I wanted to spend on this.

I don't think the comparison change here is gonna have any noticeable impact on performance tbh. In most cases the first 8 byte of the txids are already gonna differ, so the performance should be similar to comparing the pointers (assuming 8 byte addresses) most of the time.

💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1645875138)
Two (soft) reasons:
1. to make `m_script_execution_cache_hasher` `const`
2. having a separate construction function for the hasher seems more composable (e.g. testing the hasher) and makes the `ValidationCache` constructor more readable

I'm fine with your approach too, though. It addresses my main concern, everything else is more of a nit.

💬 dergoegge commented on pull request "fuzz: Improve stability for txorphan and mini_miner harnesses":
(https://github.com/bitcoin/bitcoin/pull/30306#discussion_r1645876505)
It's part of the output from my fuzzing setup, which is closed source atm.

However, measuring stability is easily possible by just running afll++.

💬 m3dwards commented on issue "ci: Move more tasks to GHA?":
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2178342409)
I didn't know PR authors could re-run tasks on Cirrus.

It is nice that you can run the jobs on your own fork, I quite often now just push a random commit to my fork to trigger the CI jobs as an experiment.

Conceivably the jobs could be on both Cirrus and GHA and only run on GHA for forks. Extra maintenance burden probably not worth it though.

How are the depends artefacts cached on Cirrus? And which docker images are you referring to? The CI build one?

Happy to volunteer to move more
...

💬 maflcko commented on issue "ci: Move more tasks to GHA?":
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2178364258)
> How are the depends artefacts cached on Cirrus? And which docker images are you referring to? The CI build one?

Cirrus itself has a simple and easy to use `cache` instruction. However, currently, the cache is implicit, because persistent workers are used.

With images I mean the ones listed by `podman image ls`, that is:


```
REPOSITORY TAG IMAGE ID CREATED SIZE
localhost/ci_native_asan latest 582be28
...

👍 willcl-ark approved a pull request: "chainparams: Add achow101 DNS seeder"
(https://github.com/bitcoin/bitcoin/pull/30007#pullrequestreview-2127870940)
reACK 2721d64989c2b2114890586b7efd01ab4b062ca6

Retested that the seed on the new domain for each chain is returning good addresses.

💬 hebasto commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2178380700)
Is something required to undraft this PR?

💬 fanquake commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2178382324)
We aren't getting much from master, so this is now just a switch to CMake. Once this lands, with CMake, a (minimal) depends + Core build will be possible entirely without autotools.

💬 m3dwards commented on issue "ci: Move more tasks to GHA?":
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2178391045)
Could we use this to cache the images? https://docs.docker.com/build/cache/backends/gha/

We are using the GHA cache at the moment, is there a reason why this woudln't work for depends? Or is it just the effort required to split up the current CI script into different steps to take advantage of GHA cache?

💬 maflcko commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1645950272)
https://github.com/bitcoin/bitcoin/actions/runs/9579271766/job/26411361263#step:7:12

```
Warning: Path Validation Error: Path(s) specified in the action for caching do(es) not exist, hence no cache is being saved.

💬 maflcko commented on issue "ci: Move more tasks to GHA?":
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2178413548)
> We are using the GHA cache at the moment, is there a reason why this woudln't work for depends?

It has a limit of 10 GB, so I am not sure if it can fit everything. https://github.com/bitcoin/bitcoin/actions/caches

💬 willcl-ark commented on issue "ci: Move more tasks to GHA?":
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2178426349)
I actually meant to ask this in #30193, but why do we cache using run-id in the key `${{ github.job }}-ccache-${{ github.run_id }}` ? As we only cache on master, using only `${{ github.job }}-ccache` would make more sense to me; a single rolling cache per job.

When we search for the cache to load we use a "wildcard" restore `restore-keys: ${{ github.job }}-ccache-` (with no run_id).

This would remove some "duplicates", e.g "macos-native-x86_64-ccache-" has 3 cache entries, when it only nee
...

💬 maflcko commented on issue "ci: Move more tasks to GHA?":
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2178432910)
> Am I missing some reason for doing things this way?

See https://github.com/bitcoin/bitcoin/pull/28292#discussion_r1298058749 . This is one of the reasons why I personally don't like GHA: It is a closed, confusing, and brittle ecosystem. The only benefit is that it is free (for now).

📝 maflcko opened a pull request: "fuzz: Fix wallet_bdb_parser 32-bit unhandled fseek error"
(https://github.com/bitcoin/bitcoin/pull/30307)
`std::fseek` on 64-bit past the end of the file may work fine (the following read would fail). However, on 32-bit it may fail early.

Fix it, by ignoring the error, treating it similar to a read error.

🤔 glozow reviewed a pull request: "test: Validate oversized transactions or without inputs"
(https://github.com/bitcoin/bitcoin/pull/29862#pullrequestreview-2127984342)
utACK 969e047cfbab86e5819a2c9056e8d2dab17513a8

Last commit looks irrelevant to the added test and can be dropped.

💬 paplorinc commented on pull request "test: Validate oversized transactions or without inputs":
(https://github.com/bitcoin/bitcoin/pull/29862#issuecomment-2178484396)
> Last commit looks irrelevant to the added test and can be dropped.

Thanks for the review! The last commit is touching the existing `bad-txns-oversize` equivalent in Python - do you want me drop it?