💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955088750)
Makes sense thanks.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955088750)
Makes sense thanks.
💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955089683)
nit: There is still one remaining in line 180.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955089683)
nit: There is still one remaining in line 180.
💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955090027)
Yeah something like that that
In most of the fuzz test in `src/test/fuzz/cluster_linearize.cpp` you provided a really nice description , which is just an overview of what the test is going to do before jumping to the instructions.
Here we just jump into the instructions without that context.
But this is nitty, you can ignore this comment and resolve.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955090027)
Yeah something like that that
In most of the fuzz test in `src/test/fuzz/cluster_linearize.cpp` you provided a really nice description , which is just an overview of what the test is going to do before jumping to the instructions.
Here we just jump into the instructions without that context.
But this is nitty, you can ignore this comment and resolve.
💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955090136)
Yeah I was thinking something like
<details>
<summary>diff</summary>
```diff
diff --git a/src/txgraph.cpp b/src/txgraph.cpp
index afaa46e6326..a9b3e446d26 100644
--- a/src/txgraph.cpp
+++ b/src/txgraph.cpp
@@ -1143,6 +1143,7 @@ void Cluster::SanityCheck(const TxGraphImpl& graph) const
// Verify m_linearization.
SetType m_done;
+ SetType last_chunk;
assert(m_depgraph.IsAcyclic());
for (auto lin_pos : m_linearization) {
assert(lin_pos < m_mappin
...
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1955090136)
Yeah I was thinking something like
<details>
<summary>diff</summary>
```diff
diff --git a/src/txgraph.cpp b/src/txgraph.cpp
index afaa46e6326..a9b3e446d26 100644
--- a/src/txgraph.cpp
+++ b/src/txgraph.cpp
@@ -1143,6 +1143,7 @@ void Cluster::SanityCheck(const TxGraphImpl& graph) const
// Verify m_linearization.
SetType m_done;
+ SetType last_chunk;
assert(m_depgraph.IsAcyclic());
for (auto lin_pos : m_linearization) {
assert(lin_pos < m_mappin
...
💬 theuni commented on pull request "depends: set `CMAKE_*_COMPILER_TARGET` in toolchain":
(https://github.com/bitcoin/bitcoin/pull/31849#issuecomment-2657542305)
Concept ACK. Regarding the duplicate `--target`... this kind of thing has come up a few times now where we need to add something to cc/flags for depends but we don't want it exported in our toolchain. I have a branch where I've done something similar.
Additionally, for Xcode generators (and CMake 4.0), I think we'll want to handle the macOS sdk/include paths differently between depends and the toolchain file, since CMake has specific variables that it really wants set for handling those thing
...
(https://github.com/bitcoin/bitcoin/pull/31849#issuecomment-2657542305)
Concept ACK. Regarding the duplicate `--target`... this kind of thing has come up a few times now where we need to add something to cc/flags for depends but we don't want it exported in our toolchain. I have a branch where I've done something similar.
Additionally, for Xcode generators (and CMake 4.0), I think we'll want to handle the macOS sdk/include paths differently between depends and the toolchain file, since CMake has specific variables that it really wants set for handling those thing
...
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1955115023)
I made a variant of this that's a bit more in line with #30635; during startup we'll wait by using the 0 hash.
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1955115023)
I made a variant of this that's a bit more in line with #30635; during startup we'll wait by using the 0 hash.
💬 theuni commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2657580903)
Hmm, I now see that @maflcko is still using the gcc coverage, so nuking it would break https://maflcko.github.io/b-c-cov/ .
@maflcko since you weren't on today's v29 milestone call: the consensus was to nuke the current gcc coverage support for v29 since it's "not working", and try to get llvm support in in its place, but that part wouldn't be a v29 release blocker.
But apparently it's working enough for you so I suspect you'd disagree with that?
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2657580903)
Hmm, I now see that @maflcko is still using the gcc coverage, so nuking it would break https://maflcko.github.io/b-c-cov/ .
@maflcko since you weren't on today's v29 milestone call: the consensus was to nuke the current gcc coverage support for v29 since it's "not working", and try to get llvm support in in its place, but that part wouldn't be a v29 release blocker.
But apparently it's working enough for you so I suspect you'd disagree with that?
👍 theuni approved a pull request: "depends: avoid an unset `CMAKE_OBJDUMP`"
(https://github.com/bitcoin/bitcoin/pull/31857#pullrequestreview-2616087124)
utACK 2434aeab62ba07c5380112838f3600b3dbbceef2
(https://github.com/bitcoin/bitcoin/pull/31857#pullrequestreview-2616087124)
utACK 2434aeab62ba07c5380112838f3600b3dbbceef2
💬 jonasschnelli commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2657584112)
I haven't looked at this closely, but it could be that the implementation needs an update.
The important point is *fingerprinting resistance*.
During designing the BIP, we decided that LIMITED NODES should always cut off at HEAD-288 to avoid getting fingerprinted by leaking the prune depth.
See the BIP:
> Counter-measures for peer fingerprinting
>
> Peers may have different prune depths (depending on the peers configuration, disk space, etc.) which can result in a fingerprinting wea
...
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2657584112)
I haven't looked at this closely, but it could be that the implementation needs an update.
The important point is *fingerprinting resistance*.
During designing the BIP, we decided that LIMITED NODES should always cut off at HEAD-288 to avoid getting fingerprinted by leaking the prune depth.
See the BIP:
> Counter-measures for peer fingerprinting
>
> Peers may have different prune depths (depending on the peers configuration, disk space, etc.) which can result in a fingerprinting wea
...
💬 darosior commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1955143126)
Why? If we are interrupted the `while()` clause above will continue here and we won't tell the router to remove the mappings and just return instead.
(https://github.com/bitcoin/bitcoin/pull/31157#discussion_r1955143126)
Why? If we are interrupted the `while()` clause above will continue here and we won't tell the router to remove the mappings and just return instead.
💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1955147830)
This still needs `LDFLAGS ${SANITIZER_LDFLAGS}` as before in order for the check to pass, so I think the `sanitizer_interface` changes above should be undone.
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1955147830)
This still needs `LDFLAGS ${SANITIZER_LDFLAGS}` as before in order for the check to pass, so I think the `sanitizer_interface` changes above should be undone.
💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2657611235)
@jonasschnelli Good point. Yes. A BIP159 node MUST be able to serve at least the latest 288 blocks, but SHOULD not serve more. Will update.
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2657611235)
@jonasschnelli Good point. Yes. A BIP159 node MUST be able to serve at least the latest 288 blocks, but SHOULD not serve more. Will update.
💬 darosior commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2657611766)
Thanks for the upload, i'll test the Mac ARM this weekend.
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2657611766)
Thanks for the upload, i'll test the Mac ARM this weekend.
💬 murchandamus commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2657614810)
Nice catch, @furszy. Thanks!
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2657614810)
Nice catch, @furszy. Thanks!
👍 hodlinator approved a pull request: "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior"
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2616050635)
re-ACK a85e8c0e6158fad2408bda5cb1e36da707eb081b
Resolved trivial conflict with #31767 since [my other ACK](https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2546259926).
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2616050635)
re-ACK a85e8c0e6158fad2408bda5cb1e36da707eb081b
Resolved trivial conflict with #31767 since [my other ACK](https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2546259926).
💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1955153140)
GoogleTest which seems like a slightly more desirable framework than Boost Unit Tests also uses [`ASSERT_*`-style](https://google.github.io/googletest/reference/assertions.html) statements I realize, so maybe keeping with that term is good in the long run.
What we're missing may be generic `assert_true(x)` & others that can be used where the special Python `assert x` feature is a bad fit.
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1955153140)
GoogleTest which seems like a slightly more desirable framework than Boost Unit Tests also uses [`ASSERT_*`-style](https://google.github.io/googletest/reference/assertions.html) statements I realize, so maybe keeping with that term is good in the long run.
What we're missing may be generic `assert_true(x)` & others that can be used where the special Python `assert x` feature is a bad fit.
💬 theuni commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2657633117)
Summarizing:
Ignoring the current impl details, the main question to be answered here is: Do we consider `-O3` to be harmful/dangerous. A ~decade ago it was commonly accepted that compiler bugs/UB were more likely to be exposed at that level, but I'm not sure how relevant that is these days.
Historically we've defaulted everything to `-O2`, but if a user set their CXXFLAGS to `-O3` then that's what they'd get. It would've been intentional.
It's a bit different with CMake because the default s
...
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2657633117)
Summarizing:
Ignoring the current impl details, the main question to be answered here is: Do we consider `-O3` to be harmful/dangerous. A ~decade ago it was commonly accepted that compiler bugs/UB were more likely to be exposed at that level, but I'm not sure how relevant that is these days.
Historically we've defaulted everything to `-O2`, but if a user set their CXXFLAGS to `-O3` then that's what they'd get. It would've been intentional.
It's a bit different with CMake because the default s
...
👍 willcl-ark approved a pull request: "cmake: Improve compatibility with Python version managers"
(https://github.com/bitcoin/bitcoin/pull/31421#pullrequestreview-2616146538)
ACK dead9086543671b07e6ce041821e4d2a6627075b
Tested with `uv` and `pyenv` on linux and macOS.
(https://github.com/bitcoin/bitcoin/pull/31421#pullrequestreview-2616146538)
ACK dead9086543671b07e6ce041821e4d2a6627075b
Tested with `uv` and `pyenv` on linux and macOS.
💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955172547)
It was a left-over of a previous approach and I left it in initially in case it might be interesting for testing, experiments etc. just because it was already there. But the PR has grown in scope a lot and it's becoming annoying to deal with for me and reviewers, so I dropped it now.
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955172547)
It was a left-over of a previous approach and I left it in initially in case it might be interesting for testing, experiments etc. just because it was already there. But the PR has grown in scope a lot and it's becoming annoying to deal with for me and reviewers, so I dropped it now.
💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955172814)
done
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955172814)
done