💬 instagibbs commented on pull request "[28.x] backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/30959#issuecomment-2386532888)
LGTM https://github.com/bitcoin/bitcoin/pull/30959/commits/5de225f5c145368f70cb5f870933bcf9df6b92c8
(https://github.com/bitcoin/bitcoin/pull/30959#issuecomment-2386532888)
LGTM https://github.com/bitcoin/bitcoin/pull/30959/commits/5de225f5c145368f70cb5f870933bcf9df6b92c8
💬 sipa commented on pull request "[28.x] backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/30959#issuecomment-2386545831)
utACK 5de225f5c145368f70cb5f870933bcf9df6b92c8
(https://github.com/bitcoin/bitcoin/pull/30959#issuecomment-2386545831)
utACK 5de225f5c145368f70cb5f870933bcf9df6b92c8
💬 dergoegge commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1783229736)
`CPPFLAGS` will have to include `-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` when building bitcoind for fuzzing with the honggfuzz netdriver.
(https://github.com/bitcoin/bitcoin/pull/31012#discussion_r1783229736)
`CPPFLAGS` will have to include `-DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` when building bitcoind for fuzzing with the honggfuzz netdriver.
🚀 achow101 merged a pull request: "[28.x] backports and finalize"
(https://github.com/bitcoin/bitcoin/pull/30959)
(https://github.com/bitcoin/bitcoin/pull/30959)
💬 glozow commented on pull request "[28.x] backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/30959#issuecomment-2386592294)
post merge ACK
(https://github.com/bitcoin/bitcoin/pull/30959#issuecomment-2386592294)
post merge ACK
📝 laanwj opened a pull request: "depends: For mingw cross compile use -gcc-posix to prevent library conflict"
(https://github.com/bitcoin/bitcoin/pull/31013)
CMake parses some paths from the spec of the C compiler, assuming it will be the linker, resulting in the link to end up with `-L/usr/lib/gcc/x86_64-w64-mingw32/12-win32` on debian bookworm if both `-win32` and `-posix` variants are installed, and `-win32` is the default alternative.
This results in the wrong C++ library being linked, missing std::threads::hardware_concurrency and other threading functions.
To fix this, use the `-posix` variant of gcc as well when available. This fixes a
...
(https://github.com/bitcoin/bitcoin/pull/31013)
CMake parses some paths from the spec of the C compiler, assuming it will be the linker, resulting in the link to end up with `-L/usr/lib/gcc/x86_64-w64-mingw32/12-win32` on debian bookworm if both `-win32` and `-posix` variants are installed, and `-win32` is the default alternative.
This results in the wrong C++ library being linked, missing std::threads::hardware_concurrency and other threading functions.
To fix this, use the `-posix` variant of gcc as well when available. This fixes a
...
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783268585)
I believe it's very rare based on our filter parameters. Each generation has 60k entries expected, and according to my debug-fu has 20 hash functions, and 21,56,646 filter bits. Since the harness does up to 10k iterations(let's assume each iteration makes a unique tx), that is 10k transactions, this leads to:
```
P_fp = (1 - e^-(20*10000/2156646))^20
~= 8.8183022e-22
```
This assumes independence of probabilities of each bit being set. Once we get up to 60k iterations, assuming
...
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783268585)
I believe it's very rare based on our filter parameters. Each generation has 60k entries expected, and according to my debug-fu has 20 hash functions, and 21,56,646 filter bits. Since the harness does up to 10k iterations(let's assume each iteration makes a unique tx), that is 10k transactions, this leads to:
```
P_fp = (1 - e^-(20*10000/2156646))^20
~= 8.8183022e-22
```
This assumes independence of probabilities of each bit being set. Once we get up to 60k iterations, assuming
...
💬 furszy commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2386629142)
> > the next step should be revert #26564?.
>
> IIUC the motivation for 26564 was to have a fully static and fixed path for the unit test datadir. My understanding is that changing the temp storage device was just a side-effect and not the primary motivation. (My comment was just a note to say that your goal is already achievable today)
Practically speaking, we're ultimately talking about pretty much the same feature. It's about customizing the test root directory path so it can be inspect
...
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2386629142)
> > the next step should be revert #26564?.
>
> IIUC the motivation for 26564 was to have a fully static and fixed path for the unit test datadir. My understanding is that changing the temp storage device was just a side-effect and not the primary motivation. (My comment was just a note to say that your goal is already achievable today)
Practically speaking, we're ultimately talking about pretty much the same feature. It's about customizing the test root directory path so it can be inspect
...
🤔 pablomartin4btc reviewed a pull request: "doc: add testnet4 section header for config file"
(https://github.com/bitcoin/bitcoin/pull/31007#pullrequestreview-2341047524)
ACK 61cdb1c9d83778b95f4f9596f34617b7a191d0a5
`BUILDDIR` would need to be updated in `contrib/devtools/gen-bitcoin-conf.sh`, otherwise `bitcoind` won't be found.
(https://github.com/bitcoin/bitcoin/pull/31007#pullrequestreview-2341047524)
ACK 61cdb1c9d83778b95f4f9596f34617b7a191d0a5
`BUILDDIR` would need to be updated in `contrib/devtools/gen-bitcoin-conf.sh`, otherwise `bitcoind` won't be found.
💬 laanwj commented on pull request "refactor: move util/pcp and util/netif to common/":
(https://github.com/bitcoin/bitcoin/pull/31011#issuecomment-2386669462)
Untested ACK fd38711217cafbd62e8abd22d2b43f85fede8cde
> The libraries.md document is a bit vague on what belongs to util vs common. It just says "low level"
Yes, i truly had no idea what to do here. Glad the CI catches it after this.
(https://github.com/bitcoin/bitcoin/pull/31011#issuecomment-2386669462)
Untested ACK fd38711217cafbd62e8abd22d2b43f85fede8cde
> The libraries.md document is a bit vague on what belongs to util vs common. It just says "low level"
Yes, i truly had no idea what to do here. Glad the CI catches it after this.
👍 sdaftuar approved a pull request: "cluster mempool: extend DepGraph functionality"
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2319841829)
Code review ACK, just some nits.
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2319841829)
Code review ACK, just some nits.
💬 sdaftuar commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1769600192)
nit: Should say "is **to** be inserted"?
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1769600192)
nit: Should say "is **to** be inserted"?
💬 sdaftuar commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1783263904)
nit: `position_range` will always just be updated to `i+1` at each loop iteration, so we could simplify this if we wanted to.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1783263904)
nit: `position_range` will always just be updated to `i+1` at each loop iteration, so we could simplify this if we wanted to.
💬 sdaftuar commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1783088965)
Just to verify my understanding: I was wondering why this loop was just up to `depgraph.TxCount()` and not `depgraph.PositionRange()`. I think this is ok because when we instantiate a new DepGraph using a reordering, we only look at the mapping positions for entries that are in `m_used`, so it doesn't matter how the unused entries are mapped at all. Is that correct?
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1783088965)
Just to verify my understanding: I was wondering why this loop was just up to `depgraph.TxCount()` and not `depgraph.PositionRange()`. I think this is ok because when we instantiate a new DepGraph using a reordering, we only look at the mapping positions for entries that are in `m_used`, so it doesn't matter how the unused entries are mapped at all. Is that correct?
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1783323170)
Indeed. Will address if I retouch.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1783323170)
Indeed. Will address if I retouch.
💬 stickies-v commented on pull request "[28.x] backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/30959#issuecomment-2386705609)
Thanks! I've added https://github.com/bitcoin-core/bitcoin-devwiki/wiki/28.0-Release-Notes-Draft/6677e1b3aa3ecadd142786f1393d2a7affe9e03e which adds missing PR numbers.
Release notes otherwise LGTM, I verified the author list and read through the descriptions. I think these 3 might benefit from being added to the list too, I'll give them a go later:
- bitcoin/bitcoin#29648
- bitcoin/bitcoin#28981
- bitcoin/bitcoin#28280
(https://github.com/bitcoin/bitcoin/pull/30959#issuecomment-2386705609)
Thanks! I've added https://github.com/bitcoin-core/bitcoin-devwiki/wiki/28.0-Release-Notes-Draft/6677e1b3aa3ecadd142786f1393d2a7affe9e03e which adds missing PR numbers.
Release notes otherwise LGTM, I verified the author list and read through the descriptions. I think these 3 might benefit from being added to the list too, I'll give them a go later:
- bitcoin/bitcoin#29648
- bitcoin/bitcoin#28981
- bitcoin/bitcoin#28280
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1783331508)
Indeed. I think this may actually have been an oversight, but but it is correct: only the first `depgraph.TxCount()` positions within `m_sorted_to_original` are actually ever accessed (because the holes are all moved to the end), and similarly, only the used positions within `m_original_to_sorted` matter.
This made me realize that this code can be improved slightly. Will do if I retouch:
```diff
--- a/src/cluster_linearize.h
+++ b/src/cluster_linearize.h
@@ -668,27 +668,24 @@ public:
...
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1783331508)
Indeed. I think this may actually have been an oversight, but but it is correct: only the first `depgraph.TxCount()` positions within `m_sorted_to_original` are actually ever accessed (because the holes are all moved to the end), and similarly, only the used positions within `m_original_to_sorted` matter.
This made me realize that this code can be improved slightly. Will do if I retouch:
```diff
--- a/src/cluster_linearize.h
+++ b/src/cluster_linearize.h
@@ -668,27 +668,24 @@ public:
...
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1783333542)
Indeed, will do if I retouch.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1783333542)
Indeed, will do if I retouch.
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1783334044)
Perhaps that can be a separate change to this then? Fixed so that change is not breaking.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1783334044)
Perhaps that can be a separate change to this then? Fixed so that change is not breaking.
💬 hebasto commented on pull request "depends: For mingw cross compile use -gcc-posix to prevent library conflict":
(https://github.com/bitcoin/bitcoin/pull/31013#issuecomment-2386727633)
Here is a related upstream issue: https://gitlab.kitware.com/cmake/cmake/-/issues/25670.
The regression is behavior is caused by setting `CMAKE_TRY_COMPILE_TARGET_TYPE` globally:https://github.com/bitcoin/bitcoin/blob/fc642c33ef28829eda0119a0fe39fd9bc4b84051/cmake/module/CheckSourceCompilesAndLinks.cmake#L9-L10
(https://github.com/bitcoin/bitcoin/pull/31013#issuecomment-2386727633)
Here is a related upstream issue: https://gitlab.kitware.com/cmake/cmake/-/issues/25670.
The regression is behavior is caused by setting `CMAKE_TRY_COMPILE_TARGET_TYPE` globally:https://github.com/bitcoin/bitcoin/blob/fc642c33ef28829eda0119a0fe39fd9bc4b84051/cmake/module/CheckSourceCompilesAndLinks.cmake#L9-L10