💬 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
💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2386732584)
> > The only exception may be the msan build, where the compiler is compiled.
> > (Just leaving a reply for context, in case someone reads the thread and stumbles upon this)
>
> Oh great, that will help simplify things a little then, as we can use more defaults. Not sure what I must've been looking at to come to the wrong conclusion here, but thanks for double-checking.
It isn't wrong. I presume it may still have to be set for the msan task, but this can probably be done in a follow-up an
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2386732584)
> > The only exception may be the msan build, where the compiler is compiled.
> > (Just leaving a reply for context, in case someone reads the thread and stumbles upon this)
>
> Oh great, that will help simplify things a little then, as we can use more defaults. Not sure what I must've been looking at to come to the wrong conclusion here, but thanks for double-checking.
It isn't wrong. I presume it may still have to be set for the msan task, but this can probably be done in a follow-up an
...
📝 laanwj opened a pull request: "net: Use GetAdaptersAddresses to get local addresses on Windows"
(https://github.com/bitcoin/bitcoin/pull/31014)
Instead of a `gethostname` hack, use the official way of calling `GetAdaptersAddresses` to get local network addresses on Windows.
Suggested by Ava Chow.
Addiional changes:
- Add optional length checking to `CService::SetSockAddr` calls: In almost all cases (the only exception is `getifaddrs`), we know the size of the data passed into `SetSockAddr`, so we can check this to be what is expected, before reading the memory.
- Cleanup: move out `FromSockAddr` in `netif.cpp` from MacOS and u
...
(https://github.com/bitcoin/bitcoin/pull/31014)
Instead of a `gethostname` hack, use the official way of calling `GetAdaptersAddresses` to get local network addresses on Windows.
Suggested by Ava Chow.
Addiional changes:
- Add optional length checking to `CService::SetSockAddr` calls: In almost all cases (the only exception is `getifaddrs`), we know the size of the data passed into `SetSockAddr`, so we can check this to be what is expected, before reading the memory.
- Cleanup: move out `FromSockAddr` in `netif.cpp` from MacOS and u
...
👍 hodlinator approved a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2341163656)
re-ACK 221296c1f0917bcf4da8fa0bbc5494a28dc6b697
`git range-diff master b9aded4 221296c`
Just improving precision of filename argument description.
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2341163656)
re-ACK 221296c1f0917bcf4da8fa0bbc5494a28dc6b697
`git range-diff master b9aded4 221296c`
Just improving precision of filename argument description.
📝 theuni opened a pull request: "build: have "make test" depend on "make all""
(https://github.com/bitcoin/bitcoin/pull/31015)
See [Upstream docs](https://cmake.org/cmake/help/latest/variable/CMAKE_SKIP_TEST_ALL_DEPENDENCY.html) for specifics.
Unfortunately, this **seems to have no effect on `ctest`** :(
This brings the test -> hack -> test cycle more inline with how it worked with autotools.
With `CMAKE_SKIP_TEST_ALL_DEPENDENCY` set to FALSE, `make test` will trigger a rebuild, ensuring that test binaries are current before running them.
To test:
```
cmake -S . -B build
make -C build -j24
touch src/prim
...
(https://github.com/bitcoin/bitcoin/pull/31015)
See [Upstream docs](https://cmake.org/cmake/help/latest/variable/CMAKE_SKIP_TEST_ALL_DEPENDENCY.html) for specifics.
Unfortunately, this **seems to have no effect on `ctest`** :(
This brings the test -> hack -> test cycle more inline with how it worked with autotools.
With `CMAKE_SKIP_TEST_ALL_DEPENDENCY` set to FALSE, `make test` will trigger a rebuild, ensuring that test binaries are current before running them.
To test:
```
cmake -S . -B build
make -C build -j24
touch src/prim
...
💬 theuni commented on pull request "build: have "make test" depend on "make all"":
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2386752224)
Ping @hebasto .
Also ping @fanquake as I think you've complained about this.
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2386752224)
Ping @hebasto .
Also ping @fanquake as I think you've complained about this.
💬 hebasto commented on pull request "build: have "make test" depend on "make all"":
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2386754681)
Does this solution support parallelism in the test phase?
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2386754681)
Does this solution support parallelism in the test phase?