💬 dergoegge commented on pull request "fuzz: Improve stability for txorphan and mini_miner harnesses":
(https://github.com/bitcoin/bitcoin/pull/30306#discussion_r1645848776)
For both MiniMiner and TxOrphange, IteratorComparator is used for a std::set that hold iterators into a std::map. I don't know how std::map allocates memory internally but it doesn't seem to be one continuous vector.
The stability for `txorphan` definitely goes up by this change:
* master: http://bitcoind-fuzz.dergoegge.de:8000/bitcoin/harnesses/txorphan/stats.txt
* PR: http://bitcoind-fuzz.dergoegge.de:8000/bitcoin-itercomp/harnesses/txorphan/stats.txt
(the first column indicates afll+'
...
(https://github.com/bitcoin/bitcoin/pull/30306#discussion_r1645848776)
For both MiniMiner and TxOrphange, IteratorComparator is used for a std::set that hold iterators into a std::map. I don't know how std::map allocates memory internally but it doesn't seem to be one continuous vector.
The stability for `txorphan` definitely goes up by this change:
* master: http://bitcoind-fuzz.dergoegge.de:8000/bitcoin/harnesses/txorphan/stats.txt
* PR: http://bitcoind-fuzz.dergoegge.de:8000/bitcoin-itercomp/harnesses/txorphan/stats.txt
(the first column indicates afll+'
...
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1645849656)
The internal address is *our* internal address toward the gateway, not the gateway's address. For IPv6 this is the same as the public address, and we explicitly bind to it before communicating (`getsockname` will just get it back, you're right). For IPv4 it could be anything in the router's internal range, so we bind to INADDR_ANY and need to get it with `getsockname`.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1645849656)
The internal address is *our* internal address toward the gateway, not the gateway's address. For IPv6 this is the same as the public address, and we explicitly bind to it before communicating (`getsockname` will just get it back, you're right). For IPv4 it could be anything in the router's internal range, so we bind to INADDR_ANY and need to get it with `getsockname`.
💬 theStack commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2178286146)
> So your other approach would be to use all the CSV fields to calculate the MuHash for the entire UTXO set at block 840,000 and then compare that to the `muhash` value of `gettxoutsetinfo muhash 840000` (with `-coinstatsindex` enabled)? Happy to try if someone gives me an incantation to paste...
See the last section in this PR's description above ("Manual test instructions"). It involves starting bitcoind with `-coinstatsindex`, creating the dump via `dumptxoutset`, converting the dump to sq
...
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2178286146)
> So your other approach would be to use all the CSV fields to calculate the MuHash for the entire UTXO set at block 840,000 and then compare that to the `muhash` value of `gettxoutsetinfo muhash 840000` (with `-coinstatsindex` enabled)? Happy to try if someone gives me an incantation to paste...
See the last section in this PR's description above ("Manual test instructions"). It involves starting bitcoind with `-coinstatsindex`, creating the dump via `dumptxoutset`, converting the dump to sq
...
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1645852506)
It would be great to cut down on the verbosity here, but as this is a header, i don't think we want to use `using` there because that will leak into anything that includes it? Or am i misunderstanding?
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1645852506)
It would be great to cut down on the verbosity here, but as this is a header, i don't think we want to use `using` there because that will leak into anything that includes it? Or am i misunderstanding?
💬 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.
(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.
(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?
(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.
(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.
(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.
(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++.
(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
...
(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
...
(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.
(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?
(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.
(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?
(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.
(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
(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
...
(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
...