💬 MarcoFalke commented on pull request "ci: Use Cirrus CI dockerfile env":
(https://github.com/bitcoin/bitcoin/pull/27340#discussion_r1166684310)
It is not really something to work on, other than maybe subscribing to the linked issue and follow-up when it is closed?
(https://github.com/bitcoin/bitcoin/pull/27340#discussion_r1166684310)
It is not really something to work on, other than maybe subscribing to the linked issue and follow-up when it is closed?
💬 MarcoFalke commented on pull request "ci: Use Cirrus CI dockerfile env":
(https://github.com/bitcoin/bitcoin/pull/27340#discussion_r1166686130)
Happy to answer any questions. The cost will be displayed on every task in a badge.
(https://github.com/bitcoin/bitcoin/pull/27340#discussion_r1166686130)
Happy to answer any questions. The cost will be displayed on every task in a badge.
💬 MarcoFalke commented on pull request "ci: Use Cirrus CI dockerfile env":
(https://github.com/bitcoin/bitcoin/pull/27340#issuecomment-1508325235)
> I tried to run the ci locally using cirrus run but ran into some issues (unrelated to this PR). If you have any tips on how to test this
I can help with running the CI locally, but not how to run Cirrus locally. I don't think testing this is possible, other than looking at the CI tasks for this pull?
(https://github.com/bitcoin/bitcoin/pull/27340#issuecomment-1508325235)
> I tried to run the ci locally using cirrus run but ran into some issues (unrelated to this PR). If you have any tips on how to test this
I can help with running the CI locally, but not how to run Cirrus locally. I don't think testing this is possible, other than looking at the CI tasks for this pull?
💬 josibake commented on pull request "ci: Use Cirrus CI dockerfile env":
(https://github.com/bitcoin/bitcoin/pull/27340#issuecomment-1508329954)
> I can help with running the CI locally, but not how to run Cirrus locally. I don't think testing this is possible, other than looking at the CI tasks for this pull?
I installed the cirrus cli tool and ran `cirrus run` in the directory, which then picks up the `.cirrus.yml` file and executes with cirrus workers, but was running into a bunch of permissions issues. Not related to this PR, so I'm not going to try and troubleshoot it further
(https://github.com/bitcoin/bitcoin/pull/27340#issuecomment-1508329954)
> I can help with running the CI locally, but not how to run Cirrus locally. I don't think testing this is possible, other than looking at the CI tasks for this pull?
I installed the cirrus cli tool and ran `cirrus run` in the directory, which then picks up the `.cirrus.yml` file and executes with cirrus workers, but was running into a bunch of permissions issues. Not related to this PR, so I'm not going to try and troubleshoot it further
💬 josibake commented on pull request "verify-commits: error and exit cleanly when git is too old.":
(https://github.com/bitcoin/bitcoin/pull/27461#issuecomment-1508338068)
ACK https://github.com/bitcoin/bitcoin/pull/27461/commits/1fefcf27edcac7ebb87c1d3c68bcd9870e3ae78a
was able to verify the failure case (v2.34) and the happy case (v2.4)
(https://github.com/bitcoin/bitcoin/pull/27461#issuecomment-1508338068)
ACK https://github.com/bitcoin/bitcoin/pull/27461/commits/1fefcf27edcac7ebb87c1d3c68bcd9870e3ae78a
was able to verify the failure case (v2.34) and the happy case (v2.4)
⚠️ glozow opened an issue: "Package Relay Project Tracking"
(https://github.com/bitcoin/bitcoin/issues/27463)
### Please describe the feature you'd like to see added.
**Mempool, Policy, Validation**
- [x] #22290
- [ ] Avoid the risk of below-minrelaytxfee transactions hanging around forever in the mempool (currently only possible through reorgs and replacements after package CPFP). There are 3 options:
- Don't ever allow v1 and v2 txs below minrelaytxfee (#26933)
- Mine everything that makes it into in the mempool (default blockmintxfee to 0). Trim descendant packages <=0 fees, even whe
...
(https://github.com/bitcoin/bitcoin/issues/27463)
### Please describe the feature you'd like to see added.
**Mempool, Policy, Validation**
- [x] #22290
- [ ] Avoid the risk of below-minrelaytxfee transactions hanging around forever in the mempool (currently only possible through reorgs and replacements after package CPFP). There are 3 options:
- Don't ever allow v1 and v2 txs below minrelaytxfee (#26933)
- Mine everything that makes it into in the mempool (default blockmintxfee to 0). Trim descendant packages <=0 fees, even whe
...
💬 RandomLattice commented on pull request "Update src/secp256k1 subtree to release v0.3.1":
(https://github.com/bitcoin/bitcoin/pull/27445#issuecomment-1508355198)
> Okay, I think I understand what's going on here
Great investigation, thanks! And sorry this broke the build.
I see you already put hotfixes in https://github.com/bitcoin-core/secp256k1/pull/1276 for the autotools issue. Thanks. I can put a hotfix for the linting issue + open an issue for a longer-term fix for linting.
(https://github.com/bitcoin/bitcoin/pull/27445#issuecomment-1508355198)
> Okay, I think I understand what's going on here
Great investigation, thanks! And sorry this broke the build.
I see you already put hotfixes in https://github.com/bitcoin-core/secp256k1/pull/1276 for the autotools issue. Thanks. I can put a hotfix for the linting issue + open an issue for a longer-term fix for linting.
💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1166712133)
I see. Then, to avoid multiple tests colliding on `/tmp/socket`, maybe this:
```python
socket_path = os.path.join(tempfile.TemporaryDirectory(), "s")
```
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1166712133)
I see. Then, to avoid multiple tests colliding on `/tmp/socket`, maybe this:
```python
socket_path = os.path.join(tempfile.TemporaryDirectory(), "s")
```
💬 MarcoFalke commented on pull request "depends: fix compiling bdb with clang-16":
(https://github.com/bitcoin/bitcoin/pull/27462#issuecomment-1508365659)
Unrelated: Interesting, that probably also means we can't switch to C++20 with depends, as long as bdb is around, assuming that depends is compiled with the same flags as Bitcoin Core.
(https://github.com/bitcoin/bitcoin/pull/27462#issuecomment-1508365659)
Unrelated: Interesting, that probably also means we can't switch to C++20 with depends, as long as bdb is around, assuming that depends is compiled with the same flags as Bitcoin Core.
💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1166722759)
It begins its life with 16 bytes pre-allocated (without using `new` or `malloc()`), but this is just an optimization which can be ignored. It is supposed to behave like `std::vector`.
Something like this should work to store `std::string` in it (untested):
```cpp
std::string path = ...;
m_addr.assign(path.begin(), path.end());
```
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1166722759)
It begins its life with 16 bytes pre-allocated (without using `new` or `malloc()`), but this is just an optimization which can be ignored. It is supposed to behave like `std::vector`.
Something like this should work to store `std::string` in it (untested):
```cpp
std::string path = ...;
m_addr.assign(path.begin(), path.end());
```
💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1166745046)
Looking into `GetGroup()` I saw it returns a C++ `std::vector<unsigned char>`. We can't pass C++ `std::vector`'s in the tracepoints as the eBPF code, written in C, won't be able to read it. We can call `data()` and pass this in the tracepoint as pointer to unsigned chars (we do this with e.g. the raw P2P messages too). To read from this pointer, we also need to pass the of the length of underlying array.
I'll give this a shot in a separate branch.
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1166745046)
Looking into `GetGroup()` I saw it returns a C++ `std::vector<unsigned char>`. We can't pass C++ `std::vector`'s in the tracepoints as the eBPF code, written in C, won't be able to read it. We can call `data()` and pass this in the tracepoint as pointer to unsigned chars (we do this with e.g. the raw P2P messages too). To read from this pointer, we also need to pass the of the length of underlying array.
I'll give this a shot in a separate branch.
💬 real-or-random commented on pull request "Update src/secp256k1 subtree to release v0.3.1":
(https://github.com/bitcoin/bitcoin/pull/27445#issuecomment-1508419009)
@sipa https://github.com/bitcoin-core/secp256k1/pull/1276 has been merged, can you update this PR to include the current secp256k1 master?
(https://github.com/bitcoin/bitcoin/pull/27445#issuecomment-1508419009)
@sipa https://github.com/bitcoin-core/secp256k1/pull/1276 has been merged, can you update this PR to include the current secp256k1 master?
💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1166768178)
An alternative is to pass `GetMappedAS()` which is an `uint32_t` that will be !=0 in an ASMap future. A net group similar to Bitcoin Core's can be calculated from the peer address by consumers that need it.
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1166768178)
An alternative is to pass `GetMappedAS()` which is an `uint32_t` that will be !=0 in an ASMap future. A net group similar to Bitcoin Core's can be calculated from the peer address by consumers that need it.
💬 fanquake commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1508445672)
> Hmm, then my guess that it could be the timeout was probably wrong. Could you run the test in isolation with -- -printtoconsole and post the log?
With the branch in #27462 (which just fixes BDB compilation), if i run the TSAN CI, it fails as it has been. However if I exec into the container after it fails, and run `src/test/test_bitcoin --catch_system_errors=no -l test_suite -t blockfilter_index_tests`, this issue does not reproduce.
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1508445672)
> Hmm, then my guess that it could be the timeout was probably wrong. Could you run the test in isolation with -- -printtoconsole and post the log?
With the branch in #27462 (which just fixes BDB compilation), if i run the TSAN CI, it fails as it has been. However if I exec into the container after it fails, and run `src/test/test_bitcoin --catch_system_errors=no -l test_suite -t blockfilter_index_tests`, this issue does not reproduce.
💬 MarcoFalke commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1508450185)
Can you also try with `LD_LIBRARY_PATH="${DEPENDS_DIR}/${HOST}/lib" ./src/test/...`
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1508450185)
Can you also try with `LD_LIBRARY_PATH="${DEPENDS_DIR}/${HOST}/lib" ./src/test/...`
💬 glozow commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1166795553)
We can't because these get rejected and `info()` only works on transactions that make it into the mempool.
(https://github.com/bitcoin/bitcoin/pull/26933#discussion_r1166795553)
We can't because these get rejected and `info()` only works on transactions that make it into the mempool.
💬 glozow commented on pull request "mempool: disallow txns under min relay fee, even in packages":
(https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1508455396)
@ajtowns thanks for the feedback, I'm not really sure where to draw the line between useful and not useful but I've made a tracking issue #27463. I've marked places where I think the outcome is meaningful (e.g. "after this, we could expose on p2p and it would be safe").
(https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1508455396)
@ajtowns thanks for the feedback, I'm not really sure where to draw the line between useful and not useful but I've made a tracking issue #27463. I've marked places where I think the outcome is meaningful (e.g. "after this, we could expose on p2p and it would be safe").
💬 achow101 commented on pull request "verify-commits: error and exit cleanly when git is too old.":
(https://github.com/bitcoin/bitcoin/pull/27461#issuecomment-1508503086)
ACK 1fefcf27edcac7ebb87c1d3c68bcd9870e3ae78a
(https://github.com/bitcoin/bitcoin/pull/27461#issuecomment-1508503086)
ACK 1fefcf27edcac7ebb87c1d3c68bcd9870e3ae78a
🚀 achow101 merged a pull request: "verify-commits: error and exit cleanly when git is too old."
(https://github.com/bitcoin/bitcoin/pull/27461)
(https://github.com/bitcoin/bitcoin/pull/27461)
💬 MarcoFalke commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1508505587)
I also can *not* reproduce with:
```
time bash -c 'RUN_UNIT_TESTS_SEQUENTIAL="true" RUN_UNIT_TESTS="false" RUN_FUNCTIONAL_TESTS=false CCACHE_SIZE=500M MAKEJOBS="-j9" FILE_ENV="./ci/test/00_setup_env_native_tsan.sh" ./ci/test_run_all.sh'
```
and diff:
```diff
diff --git a/ci/test/06_script_b.sh b/ci/test/06_script_b.sh
index f7dcbcee5..899c99119 100755
--- a/ci/test/06_script_b.sh
+++ b/ci/test/06_script_b.sh
@@ -29,7 +29,7 @@ if [ "$RUN_UNIT_TESTS" = "true" ]; then
fi
if [ "
...
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1508505587)
I also can *not* reproduce with:
```
time bash -c 'RUN_UNIT_TESTS_SEQUENTIAL="true" RUN_UNIT_TESTS="false" RUN_FUNCTIONAL_TESTS=false CCACHE_SIZE=500M MAKEJOBS="-j9" FILE_ENV="./ci/test/00_setup_env_native_tsan.sh" ./ci/test_run_all.sh'
```
and diff:
```diff
diff --git a/ci/test/06_script_b.sh b/ci/test/06_script_b.sh
index f7dcbcee5..899c99119 100755
--- a/ci/test/06_script_b.sh
+++ b/ci/test/06_script_b.sh
@@ -29,7 +29,7 @@ if [ "$RUN_UNIT_TESTS" = "true" ]; then
fi
if [ "
...