💬 jonatack commented on pull request "[26.x] rc3 or finalize":
(https://github.com/bitcoin/bitcoin/pull/28872#issuecomment-1810696940)
> there is currently nothing further marked for backport to 26.x
Would suggest backporting #28155 as mentioned in https://github.com/bitcoin/bitcoin/pull/28155#issuecomment-1785999512.
Am rebasing and updating #28248 today, which fixes a number of bugs including the regression reported in https://github.com/bitcoin/bitcoin/pull/27213/files#r1291926369 and described in https://github.com/bitcoin/bitcoin/pull/28248/commits and https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-178970
...
(https://github.com/bitcoin/bitcoin/pull/28872#issuecomment-1810696940)
> there is currently nothing further marked for backport to 26.x
Would suggest backporting #28155 as mentioned in https://github.com/bitcoin/bitcoin/pull/28155#issuecomment-1785999512.
Am rebasing and updating #28248 today, which fixes a number of bugs including the regression reported in https://github.com/bitcoin/bitcoin/pull/27213/files#r1291926369 and described in https://github.com/bitcoin/bitcoin/pull/28248/commits and https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-178970
...
💬 dergoegge commented on pull request "fuzz: AutoFile with XOR":
(https://github.com/bitcoin/bitcoin/pull/28873#issuecomment-1810700861)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28873#issuecomment-1810700861)
Concept ACK
💬 dergoegge commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#issuecomment-1810702008)
Concept ACK
Thanks! will review soon
(https://github.com/bitcoin/bitcoin/pull/28875#issuecomment-1810702008)
Concept ACK
Thanks! will review soon
💬 achow101 commented on issue "Package Relay Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1810708407)
#26711 was closed. Could we get a longer explanation of why and what the plan is going forward? My understanding is that the plan is to do a limited package relay carveout for just CPFP, then cluster mempool, then the rest of package relay?
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1810708407)
#26711 was closed. Could we get a longer explanation of why and what the plan is going forward? My understanding is that the plan is to do a limited package relay carveout for just CPFP, then cluster mempool, then the rest of package relay?
💬 achow101 commented on pull request "test: Generate coverage report without running tests":
(https://github.com/bitcoin/bitcoin/pull/28772#issuecomment-1810710622)
> The docs don't mention `make check`
Hmm? It's right there in the first sentence of that section:
> LCOV can be used to generate a test coverage report based upon `make check` execution.
(https://github.com/bitcoin/bitcoin/pull/28772#issuecomment-1810710622)
> The docs don't mention `make check`
Hmm? It's right there in the first sentence of that section:
> LCOV can be used to generate a test coverage report based upon `make check` execution.
💬 jonatack commented on pull request "test: fix node index bug when comparing peerinfo":
(https://github.com/bitcoin/bitcoin/pull/28849#issuecomment-1810715496)
Good eye, @kashifs 👍
(https://github.com/bitcoin/bitcoin/pull/28849#issuecomment-1810715496)
Good eye, @kashifs 👍
💬 dergoegge commented on pull request "test: Generate coverage report without running tests":
(https://github.com/bitcoin/bitcoin/pull/28772#issuecomment-1810722197)
> Hmm? It's right there in the first sentence of that section:
>
>> LCOV can be used to generate a test coverage report based upon make check execution.
Isn't that just saying that the generated coverage report covers the code executed in `make check`, not that you have to invoke `make check` prior to `make cov`? (that's at least how i read it and how `make cov` currently works). Since there are multiple interpretations, it probably makes sense to clarify.
(https://github.com/bitcoin/bitcoin/pull/28772#issuecomment-1810722197)
> Hmm? It's right there in the first sentence of that section:
>
>> LCOV can be used to generate a test coverage report based upon make check execution.
Isn't that just saying that the generated coverage report covers the code executed in `make check`, not that you have to invoke `make check` prior to `make cov`? (that's at least how i read it and how `make cov` currently works). Since there are multiple interpretations, it probably makes sense to clarify.
🤔 stickies-v reviewed a pull request: "test: refactor: use built-in collection types for type hints (Python 3.9 / PEP 585)"
(https://github.com/bitcoin/bitcoin/pull/28725#pullrequestreview-1730313930)
Approach ACK 4b9afb18e6b9e16d7b299820f3a1382986a451d4
Reducing boilerplate for type hints is nice, and even though there's no deprecation date set, from PEP585 it appears the intent is for the `typing` types is to be deprecated in favor of the generic types.
From PEP585, it's also recommended to replace `Callable` and `Iterable` with their `collections.abc` alternative, done is this diff:
<details>
<summary>git diff on 4b9afb18e6</summary>
```diff
diff --git a/contrib/seeds/asmap.p
...
(https://github.com/bitcoin/bitcoin/pull/28725#pullrequestreview-1730313930)
Approach ACK 4b9afb18e6b9e16d7b299820f3a1382986a451d4
Reducing boilerplate for type hints is nice, and even though there's no deprecation date set, from PEP585 it appears the intent is for the `typing` types is to be deprecated in favor of the generic types.
From PEP585, it's also recommended to replace `Callable` and `Iterable` with their `collections.abc` alternative, done is this diff:
<details>
<summary>git diff on 4b9afb18e6</summary>
```diff
diff --git a/contrib/seeds/asmap.p
...
🤔 theStack reviewed a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1730319269)
I've noticed that the test `p2p_v2_encrypted.py` fails if the node under test sends a small amount of garbage (in the range of 0-19 bytes). Can be reproduced by applying the following simple patch:
```diff
diff --git a/src/net.cpp b/src/net.cpp
index 2682035912..09af15a9b7 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -951,7 +951,7 @@ std::vector<uint8_t> GenerateRandomGarbage() noexcept
{
std::vector<uint8_t> ret;
FastRandomContext rng;
- ret.resize(rng.randrange(V2Trans
...
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1730319269)
I've noticed that the test `p2p_v2_encrypted.py` fails if the node under test sends a small amount of garbage (in the range of 0-19 bytes). Can be reproduced by applying the following simple patch:
```diff
diff --git a/src/net.cpp b/src/net.cpp
index 2682035912..09af15a9b7 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -951,7 +951,7 @@ std::vector<uint8_t> GenerateRandomGarbage() noexcept
{
std::vector<uint8_t> ret;
FastRandomContext rng;
- ret.resize(rng.randrange(V2Trans
...
💬 instagibbs commented on issue "Package Relay Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1810800093)
I'll update Ephemeral Anchors PR/BIP once current set of PRs are merged, since I build on https://github.com/bitcoin/bitcoin/pull/28848 for testing
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1810800093)
I'll update Ephemeral Anchors PR/BIP once current set of PRs are merged, since I build on https://github.com/bitcoin/bitcoin/pull/28848 for testing
📝 hebasto opened a pull request: "build: Fix LTO functionality"
(https://github.com/bitcoin/bitcoin/pull/28876)
The current LTO functionality implementation has multiple issues:
1. Duplicated `-flto` flags when linking.
2. The `secp256k1` subtree code is built without LTO.
3. In `AX_CHECK_LINK_FLAG` macro the wrong `CXXFLAG_WERROR` flag is used instead of the correct `LDFLAG_WERROR` one.
This PR fixes all of them.
Might be verified as follows:
```
$ readelf -S src/secp256k1/src/libsecp256k1_la-secp256k1.o | grep .gnu.lto_ | wc -l
274
```
---
The depends build system has similar issues t
...
(https://github.com/bitcoin/bitcoin/pull/28876)
The current LTO functionality implementation has multiple issues:
1. Duplicated `-flto` flags when linking.
2. The `secp256k1` subtree code is built without LTO.
3. In `AX_CHECK_LINK_FLAG` macro the wrong `CXXFLAG_WERROR` flag is used instead of the correct `LDFLAG_WERROR` one.
This PR fixes all of them.
Might be verified as follows:
```
$ readelf -S src/secp256k1/src/libsecp256k1_la-secp256k1.o | grep .gnu.lto_ | wc -l
274
```
---
The depends build system has similar issues t
...
💬 glozow commented on issue "Package Relay Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1810836012)
There have been discussions on what package validation and RBF rules will look like in a post-cluster mempool world. #26711 either doesn't work with cluster mempool or should not be considered until after cluster mempool. #26711 supports certain types of package submission that we might not be able to continue to support with cluster mempool depending on what approach is taken. There are different ideas on how to consider subsets of packages to accept when it doesn't make sense to take all or no
...
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1810836012)
There have been discussions on what package validation and RBF rules will look like in a post-cluster mempool world. #26711 either doesn't work with cluster mempool or should not be considered until after cluster mempool. #26711 supports certain types of package submission that we might not be able to continue to support with cluster mempool depending on what approach is taken. There are different ideas on how to consider subsets of packages to accept when it doesn't make sense to take all or no
...
💬 glozow commented on issue "Package Relay Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1810839054)
The OP has been updated to reflect the new plan.
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1810839054)
The OP has been updated to reflect the new plan.
💬 jonatack commented on pull request "Implement `CCoinsViewErrorCatcher::HaveCoin` and check disk space periodically":
(https://github.com/bitcoin/bitcoin/pull/26331#issuecomment-1810843435)
Post-merge concept ACK. Some kind of test coverage would be good.
(https://github.com/bitcoin/bitcoin/pull/26331#issuecomment-1810843435)
Post-merge concept ACK. Some kind of test coverage would be good.
💬 ismaelsadeeq commented on pull request "wallet: propagete `checkChainLimits` error message to wallet":
(https://github.com/bitcoin/bitcoin/pull/28863#issuecomment-1810845253)
> It would be nice if the test can reach all four possible limit error messages, but if that's difficult to achieve it could wait for a followup.
Previosuly we always got the `Transaction has too long of a mempool chain` message but with this PR we now get the node unconfirmed parents limit.
[checkChainLimits](https://github.com/bitcoin/bitcoin/blob/5800c558eb5efb4839ed00d6967e43306d68e1c3/src/node/interfaces.cpp#L703) is sending a package with one Internally created transaction and its `v
...
(https://github.com/bitcoin/bitcoin/pull/28863#issuecomment-1810845253)
> It would be nice if the test can reach all four possible limit error messages, but if that's difficult to achieve it could wait for a followup.
Previosuly we always got the `Transaction has too long of a mempool chain` message but with this PR we now get the node unconfirmed parents limit.
[checkChainLimits](https://github.com/bitcoin/bitcoin/blob/5800c558eb5efb4839ed00d6967e43306d68e1c3/src/node/interfaces.cpp#L703) is sending a package with one Internally created transaction and its `v
...
🤔 jonatack reviewed a pull request: "build: remove `-bind_at_load` usage"
(https://github.com/bitcoin/bitcoin/pull/28783#pullrequestreview-1730409395)
Tested on ARM64 M1 Max with macOS Ventura 13.6 and the `bind_at_load` warnings are now gone. Thank you for fixing this.
(https://github.com/bitcoin/bitcoin/pull/28783#pullrequestreview-1730409395)
Tested on ARM64 M1 Max with macOS Ventura 13.6 and the `bind_at_load` warnings are now gone. Thank you for fixing this.
💬 brunoerg commented on pull request "test: add stress test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1810888521)
> Can you clarify what this meant to do? What is it stressing, where do the magic numbers "2000, 1/3, 2/3" come from, what is expected to fail under this "stress"?
Sure! "Stressing" is basically test bucketing logic in a situation that we try to add in the new table:
- Addresses from more distinct ASs than the number of available buckets (2000 is a little bit less than 2x the number of buckets. I chose this number to not make the test slower, however, if we migrate this test to `contrib`, w
...
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1810888521)
> Can you clarify what this meant to do? What is it stressing, where do the magic numbers "2000, 1/3, 2/3" come from, what is expected to fail under this "stress"?
Sure! "Stressing" is basically test bucketing logic in a situation that we try to add in the new table:
- Addresses from more distinct ASs than the number of available buckets (2000 is a little bit less than 2x the number of buckets. I chose this number to not make the test slower, however, if we migrate this test to `contrib`, w
...
💬 jonatack commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1810906067)
Here is what happened when I tried to use this according to the added documentation. Any suggestion?
```
jon|master =:~/bitcoin/bitcoin$ cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && cargo run
zsh: command not found: cargo
jon|master =:~/bitcoin/bitcoin/test/lint/test_runner$ brew install cargo
Running `brew update --auto-update`...
==> Auto-updated Homebrew!
==> Updated Homebrew from 75e8376816 to 8df40f4a41.
No changes to formulae or casks.
Warning: No available f
...
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1810906067)
Here is what happened when I tried to use this according to the added documentation. Any suggestion?
```
jon|master =:~/bitcoin/bitcoin$ cd ./test/lint/test_runner/ && cargo fmt && cargo clippy && cargo run
zsh: command not found: cargo
jon|master =:~/bitcoin/bitcoin/test/lint/test_runner$ brew install cargo
Running `brew update --auto-update`...
==> Auto-updated Homebrew!
==> Updated Homebrew from 75e8376816 to 8df40f4a41.
No changes to formulae or casks.
Warning: No available f
...
📝 TheCharlatan opened a pull request: "bench: Update nanobench to 4.3.11"
(https://github.com/bitcoin/bitcoin/pull/28877)
The newest version fixes the false positive `* Turbo is enabled, CPU frequency will fluctuate` warning on AMD CPUs. The file was directly taken from the release page: https://github.com/martinus/nanobench/releases/tag/v4.3.11.
Other changes from the release notes:
* Check for failures in parseFile(), perf events tweaks by @tommi-cujo in https://github.com/martinus/nanobench/pull/84
* Workaround missing noexcept for std::string move assignment by @tommi-cujo in https://github.com/martinus/
...
(https://github.com/bitcoin/bitcoin/pull/28877)
The newest version fixes the false positive `* Turbo is enabled, CPU frequency will fluctuate` warning on AMD CPUs. The file was directly taken from the release page: https://github.com/martinus/nanobench/releases/tag/v4.3.11.
Other changes from the release notes:
* Check for failures in parseFile(), perf events tweaks by @tommi-cujo in https://github.com/martinus/nanobench/pull/84
* Workaround missing noexcept for std::string move assignment by @tommi-cujo in https://github.com/martinus/
...
💬 fanquake commented on pull request "build: Fix LTO functionality":
(https://github.com/bitcoin/bitcoin/pull/28876#issuecomment-1811062710)
In case anyone is unsure, the --enable-lto option, as-is, does work, no broken binaries etc (and we use (thin) LTO in oss-fuzz).
Changes 1 & 3 here don't affect the compiled binary. The compiler will eat any duplicate -flto flags; the change to the -Werror flag is correct, but also does not affect the compiled binary.
Passing the flags through to libsecp is an improvement, and I'm pretty sure that wasn't done originally, because when the --enable-lto option was implemented, we didn't have a cl
...
(https://github.com/bitcoin/bitcoin/pull/28876#issuecomment-1811062710)
In case anyone is unsure, the --enable-lto option, as-is, does work, no broken binaries etc (and we use (thin) LTO in oss-fuzz).
Changes 1 & 3 here don't affect the compiled binary. The compiler will eat any duplicate -flto flags; the change to the -Werror flag is correct, but also does not affect the compiled binary.
Passing the flags through to libsecp is an improvement, and I'm pretty sure that wasn't done originally, because when the --enable-lto option was implemented, we didn't have a cl
...