💬 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?
💬 theuni commented on pull request "build: have "make test" depend on "make all"":
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2386761343)
With `ARGS=-jX`, yes.
(https://github.com/bitcoin/bitcoin/pull/31015#issuecomment-2386761343)
With `ARGS=-jX`, yes.
💬 maflcko commented on pull request "ci: Approximate MAKEJOBS in image build phase":
(https://github.com/bitcoin/bitcoin/pull/30935#issuecomment-2386766435)
This may or may not be needed for https://github.com/bitcoin/bitcoin/issues/30852, but it could be useful even outside of that, if someone wanted to set `MAKEJOBS` when building the image.
(https://github.com/bitcoin/bitcoin/pull/30935#issuecomment-2386766435)
This may or may not be needed for https://github.com/bitcoin/bitcoin/issues/30852, but it could be useful even outside of that, if someone wanted to set `MAKEJOBS` when building the image.
💬 laanwj commented on pull request "depends: For mingw cross compile use -gcc-posix to prevent library conflict":
(https://github.com/bitcoin/bitcoin/pull/31013#issuecomment-2386785047)
Yes, that seems similar! It would work perfectly fine without adding any `-L` spec, the linker knows where to find its libraries, no need to add the implicit link directories explicitly at all, but it does and it breaks.
(https://github.com/bitcoin/bitcoin/pull/31013#issuecomment-2386785047)
Yes, that seems similar! It would work perfectly fine without adding any `-L` spec, the linker knows where to find its libraries, no need to add the implicit link directories explicitly at all, but it does and it breaks.
💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2386800235)
Updated with the following changes:
- key is now `siphash(k, spent outpoint)` (8 bytes) where k is a random value created when the index is created, to defend against potential collisions attacks. Value is still a tx position (8 bytes)
- `gettxspendingprevout` will return the full spending tx if option ` include_tx` is set
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2386800235)
Updated with the following changes:
- key is now `siphash(k, spent outpoint)` (8 bytes) where k is a random value created when the index is created, to defend against potential collisions attacks. Value is still a tx position (8 bytes)
- `gettxspendingprevout` will return the full spending tx if option ` include_tx` is set
💬 willcl-ark commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2386850370)
> Apart from the inital clone+apt, the tasks are not IO-bound (at least not on Hetzner Cloud).
I know in docker you can cache apt, I wonder if we can do the same on podman (having difficulty finding that they support the exact same flags)? e.g. in docker you can:
```bash
RUN --mount=target=/var/lib/apt/lists,type=cache,sharing=locked \
--mount=target=/var/cache/apt,type=cache,sharing=locked \
```
We'd need likely to delineate between Ubuntu versions etc. and this may only help wi
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2386850370)
> Apart from the inital clone+apt, the tasks are not IO-bound (at least not on Hetzner Cloud).
I know in docker you can cache apt, I wonder if we can do the same on podman (having difficulty finding that they support the exact same flags)? e.g. in docker you can:
```bash
RUN --mount=target=/var/lib/apt/lists,type=cache,sharing=locked \
--mount=target=/var/cache/apt,type=cache,sharing=locked \
```
We'd need likely to delineate between Ubuntu versions etc. and this may only help wi
...