💬 maflcko commented on pull request "util: Catch translation string errors at compile time":
(https://github.com/bitcoin/bitcoin/pull/30383#issuecomment-2219940911)
Rebased to drop merged commit.
Current CI failure is unrelated and can be ignored.
(https://github.com/bitcoin/bitcoin/pull/30383#issuecomment-2219940911)
Rebased to drop merged commit.
Current CI failure is unrelated and can be ignored.
💬 fjahr commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1671897509)
I think it's a bit too verbose for the log but I added a comment. And I think it will be confusing to explicitly say everywhere in the normal case that the tip is an ancestor of the snapshot. But it's documented in one place now at least.
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1671897509)
I think it's a bit too verbose for the log but I added a comment. And I think it will be confusing to explicitly say everywhere in the normal case that the tip is an ancestor of the snapshot. But it's documented in one place now at least.
💬 fjahr commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1671897648)
I think it's fine to keep the function name as is. The comment (now a log) provides the necessary context and I think it's just a special edge case of the less work behavior.
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1671897648)
I think it's fine to keep the function name as is. The comment (now a log) provides the necessary context and I think it's just a special edge case of the less work behavior.
💬 fjahr commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1671897825)
Done
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1671897825)
Done
💬 fjahr commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#issuecomment-2219987968)
Rebased (caused by #30329)
(https://github.com/bitcoin/bitcoin/pull/29656#issuecomment-2219987968)
Rebased (caused by #30329)
💬 stratospher commented on issue "ci: failure in `p2p_v2_misbehaving.py`":
(https://github.com/bitcoin/bitcoin/issues/30419#issuecomment-2220039790)
oh a race condition happened because `initiate_v2_handshake()` code in `p2p_v2_misbehaving.py` was simplified. i think a simple `wait_until()` should fix it. will open a PR.
(https://github.com/bitcoin/bitcoin/issues/30419#issuecomment-2220039790)
oh a race condition happened because `initiate_v2_handshake()` code in `p2p_v2_misbehaving.py` was simplified. i think a simple `wait_until()` should fix it. will open a PR.
🤔 fjahr reviewed a pull request: "assumeUTXO: snapshot load, update VERSION msg height"
(https://github.com/bitcoin/bitcoin/pull/30418#pullrequestreview-2168560487)
I am still undecided if we want this and need to think about it a little more.
We send this usually when we have a new block but the loading of the snapshot does not necessarily mean that we have the block, right? We only need the header for that. So we should probably set it when we get block itself I think.
(https://github.com/bitcoin/bitcoin/pull/30418#pullrequestreview-2168560487)
I am still undecided if we want this and need to think about it a little more.
We send this usually when we have a new block but the loading of the snapshot does not necessarily mean that we have the block, right? We only need the header for that. So we should probably set it when we get block itself I think.
💬 fjahr commented on pull request "assumeUTXO: snapshot load, update VERSION msg height":
(https://github.com/bitcoin/bitcoin/pull/30418#discussion_r1671946079)
Are you sure this is the best place for this in the RPC? Looking at the other calls to `SetBestBlock()` they all seem to be happening in `validation.cpp`.
(https://github.com/bitcoin/bitcoin/pull/30418#discussion_r1671946079)
Are you sure this is the best place for this in the RPC? Looking at the other calls to `SetBestBlock()` they all seem to be happening in `validation.cpp`.
💬 fjahr commented on pull request "assumeUTXO: snapshot load, update VERSION msg height":
(https://github.com/bitcoin/bitcoin/pull/30418#discussion_r1671948758)
nit: You can also get the height out of `metadata` without the mutex (see below).
(https://github.com/bitcoin/bitcoin/pull/30418#discussion_r1671948758)
nit: You can also get the height out of `metadata` without the mutex (see below).
📝 stratospher opened a pull request: "test: Fix intermittent failure in p2p_v2_misbehaving.py"
(https://github.com/bitcoin/bitcoin/pull/30420)
Fixes #30419.
Make sure that ellswift computation is complete in the `NetworkThread` in `test/functional/p2p_v2_misbehaving.py` before sending the ellswift in the `MainThread`.
One way to reproduce this failure on master would be:
```diff
diff --git a/test/functional/test_framework/v2_p2p.py b/test/functional/test_framework/v2_p2p.py
index 87600c36de..ea0615ef3b 100644
--- a/test/functional/test_framework/v2_p2p.py
+++ b/test/functional/test_framework/v2_p2p.py
@@ -111,6 +111,7 @
...
(https://github.com/bitcoin/bitcoin/pull/30420)
Fixes #30419.
Make sure that ellswift computation is complete in the `NetworkThread` in `test/functional/p2p_v2_misbehaving.py` before sending the ellswift in the `MainThread`.
One way to reproduce this failure on master would be:
```diff
diff --git a/test/functional/test_framework/v2_p2p.py b/test/functional/test_framework/v2_p2p.py
index 87600c36de..ea0615ef3b 100644
--- a/test/functional/test_framework/v2_p2p.py
+++ b/test/functional/test_framework/v2_p2p.py
@@ -111,6 +111,7 @
...
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2220126461)
> I don't think that benchmark is related, could just be noise.
Indeed, after your rebase the speed seems to be the same.
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2220126461)
> I don't think that benchmark is related, could just be noise.
Indeed, after your rebase the speed seems to be the same.
🤔 paplorinc reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2168691351)
Thanks, I will try to properly test the speed in the following weeks (maybe I'll just rent a VM and do it there)
utACK 5c37dfdda55384358540cfae50715faefa79c933
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2168691351)
Thanks, I will try to properly test the speed in the following weeks (maybe I'll just rent a VM and do it there)
utACK 5c37dfdda55384358540cfae50715faefa79c933
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1672026153)
very clean diff, love it
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1672026153)
very clean diff, love it
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1672031368)
isn't the parameter naming off if `not erase`, will lead to `map.erase`?
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1672031368)
isn't the parameter naming off if `not erase`, will lead to `map.erase`?
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1672025664)
nit: if you end up modifying again, please fix the ` &` here as well
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1672025664)
nit: if you end up modifying again, please fix the ` &` here as well
💬 fjahr commented on pull request "test: Fix intermittent failure in p2p_v2_misbehaving.py":
(https://github.com/bitcoin/bitcoin/pull/30420#issuecomment-2220219067)
Concept ACK
I could verify that the issue I see with `sleep(3)` is fixed with the change here. However, when I increase the number from 3 to 5 or 10 I still see other errors, so there may be more potential for race conditions here.
The two failures I saw:
```
2024-07-10T10:56:33.201000Z TestFramework (INFO): expected now, our custom data_received() function wouldn't result in assertion failure
2024-07-10T10:56:38.167000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:15258
...
(https://github.com/bitcoin/bitcoin/pull/30420#issuecomment-2220219067)
Concept ACK
I could verify that the issue I see with `sleep(3)` is fixed with the change here. However, when I increase the number from 3 to 5 or 10 I still see other errors, so there may be more potential for race conditions here.
The two failures I saw:
```
2024-07-10T10:56:33.201000Z TestFramework (INFO): expected now, our custom data_received() function wouldn't result in assertion failure
2024-07-10T10:56:38.167000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:15258
...
💬 glozow commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2220223954)
post merge ACK. My local bench results had comparable performance as well
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2220223954)
post merge ACK. My local bench results had comparable performance as well
💬 glozow commented on pull request "[doc] archive v26.2 release notes":
(https://github.com/bitcoin/bitcoin/pull/30414#issuecomment-2220242693)
Can be reviewed with `git diff 3c61cf3:doc/release-notes/release-notes-26.2.md v26.2:doc/release-notes.md`
(https://github.com/bitcoin/bitcoin/pull/30414#issuecomment-2220242693)
Can be reviewed with `git diff 3c61cf3:doc/release-notes/release-notes-26.2.md v26.2:doc/release-notes.md`
💬 fanquake commented on pull request "guix: use GCC 13 to builds releases":
(https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1672103392)
Re-added. (unrelated, looks like the win64 CI is randomly failing once again).
(https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1672103392)
Re-added. (unrelated, looks like the win64 CI is randomly failing once again).
👍 stickies-v approved a pull request: "[doc] archive v26.2 release notes"
(https://github.com/bitcoin/bitcoin/pull/30414#pullrequestreview-2168833104)
ACK 3c61cf3986d40ce0eae794f8a9571247f8af99e4
(https://github.com/bitcoin/bitcoin/pull/30414#pullrequestreview-2168833104)
ACK 3c61cf3986d40ce0eae794f8a9571247f8af99e4