💬 fanquake commented on pull request "upnp: fix build with miniupnpc 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2178174014)
Backports for 27.x in #30305.
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2178174014)
Backports for 27.x in #30305.
📝 dergoegge opened a pull request: "fuzz: Improve stability for txorphan and mini_miner harnesses"
(https://github.com/bitcoin/bitcoin/pull/30306)
See #29018.
Stability for `txorphan` is now >90%. `mini_miner` needs further investigation, stability still low (although slightly improved by this PR) at ~62%.
(https://github.com/bitcoin/bitcoin/pull/30306)
See #29018.
Stability for `txorphan` is now >90%. `mini_miner` needs further investigation, stability still low (although slightly improved by this PR) at ~62%.
🤔 glozow reviewed a pull request: "Fee Estimation: change `estimatesmartfee` default mode to `economical`"
(https://github.com/bitcoin/bitcoin/pull/30275#pullrequestreview-2127681911)
Concept ACK 5a4a84a6809919561aec46f272f08ae57c0e386e. Slightly surprised no tests need to be changed.
Aside from seeming to overestimate much less, I think "economical" is much closer to what users would expect from a fee estimator. From #10589:
> The logic used here is that any time a transaction signals opt-in-RBF and uses automatic fee estimation then it will use the non-conservative estimate. Transactions which do not signal opt-in-RBF will still use the default conservative estimate.
...
(https://github.com/bitcoin/bitcoin/pull/30275#pullrequestreview-2127681911)
Concept ACK 5a4a84a6809919561aec46f272f08ae57c0e386e. Slightly surprised no tests need to be changed.
Aside from seeming to overestimate much less, I think "economical" is much closer to what users would expect from a fee estimator. From #10589:
> The logic used here is that any time a transaction signals opt-in-RBF and uses automatic fee estimation then it will use the non-conservative estimate. Transactions which do not signal opt-in-RBF will still use the default conservative estimate.
...
💬 MkpoikankeAbasi commented on issue "ci: Move more tasks to GHA?":
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2178243471)
Due to infrastructure send bitcoin to confirm this address 1CnirRAFdzyjo2LHn8t7tK976bEnWFwcFS
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2178243471)
Due to infrastructure send bitcoin to confirm this address 1CnirRAFdzyjo2LHn8t7tK976bEnWFwcFS
🤔 maflcko reviewed a pull request: "fuzz: Improve stability for txorphan and mini_miner harnesses"
(https://github.com/bitcoin/bitcoin/pull/30306#pullrequestreview-2127716536)
left some questions
(https://github.com/bitcoin/bitcoin/pull/30306#pullrequestreview-2127716536)
left some questions
💬 maflcko commented on pull request "fuzz: Improve stability for txorphan and mini_miner harnesses":
(https://github.com/bitcoin/bitcoin/pull/30306#discussion_r1645834791)
I wonder if the set could just be made a vector, because duplicates shouldn't happen?
(https://github.com/bitcoin/bitcoin/pull/30306#discussion_r1645834791)
I wonder if the set could just be made a vector, because duplicates shouldn't happen?
💬 maflcko commented on pull request "fuzz: Improve stability for txorphan and mini_miner harnesses":
(https://github.com/bitcoin/bitcoin/pull/30306#discussion_r1645822565)
Can you explain this a bit more? IIUC the entries/iterators are pushed into a vector, so the order should be deterministic, no? If not, maybe the order should be made deterministic? Otherwise, the hot-loop may of calculating the ancestor set may take a longer time due to having to compare more memory (pointer vs uint256)?
(https://github.com/bitcoin/bitcoin/pull/30306#discussion_r1645822565)
Can you explain this a bit more? IIUC the entries/iterators are pushed into a vector, so the order should be deterministic, no? If not, maybe the order should be made deterministic? Otherwise, the hot-loop may of calculating the ancestor set may take a longer time due to having to compare more memory (pointer vs uint256)?
💬 maflcko commented on issue "fuzz: Fix stability, determinism issues":
(https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-2178266350)
Looks like there is currently no data for the last week. https://oss-fuzz.com/fuzzer-stats?group_by=by-fuzzer&date_start=2024-06-12&date_end=2024-06-18&fuzzer=afl&job=afl_asan_bitcoin-core&project=bitcoin-core returns a 404
(https://github.com/bitcoin/bitcoin/issues/29018#issuecomment-2178266350)
Looks like there is currently no data for the last week. https://oss-fuzz.com/fuzzer-stats?group_by=by-fuzzer&date_start=2024-06-12&date_end=2024-06-18&fuzzer=afl&job=afl_asan_bitcoin-core&project=bitcoin-core returns a 404
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1645841553)
sure, we could do that again, but i don't want people to have to bother with weird error messages and having to load kernel modules, probably this should work out of the box or not at all
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1645841553)
sure, we could do that again, but i don't want people to have to bother with weird error messages and having to load kernel modules, probably this should work out of the box or not at all
💬 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++.