💬 MarcoFalke commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1568815816)
Looks like the fuzz test needs an update?
```
82bb7831fa6052620998c7eef47e48ed594248a8 is the first bad commit
```
```
echo 'AQAAAAEAAAA='|base64 --decode > /tmp/a
FUZZ=wallet_notifications ./src/test/fuzz/fuzz /tmp/a
```
```
fuzz: wallet/test/fuzz/notifications.cpp:175: void wallet::(anonymous namespace)::wallet_notifications_fuzz_target(FuzzBufferType): Assertion `total_amount == bal_a + bal_b' failed.
==301525== ERROR: libFuzzer: deadly signal
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1568815816)
Looks like the fuzz test needs an update?
```
82bb7831fa6052620998c7eef47e48ed594248a8 is the first bad commit
```
```
echo 'AQAAAAEAAAA='|base64 --decode > /tmp/a
FUZZ=wallet_notifications ./src/test/fuzz/fuzz /tmp/a
```
```
fuzz: wallet/test/fuzz/notifications.cpp:175: void wallet::(anonymous namespace)::wallet_notifications_fuzz_target(FuzzBufferType): Assertion `total_amount == bal_a + bal_b' failed.
==301525== ERROR: libFuzzer: deadly signal
🤔 stickies-v reviewed a pull request: "ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN"
(https://github.com/bitcoin/bitcoin/pull/27777#pullrequestreview-1451472783)
ACK fa9c65a74cf18e9c75cd3472112d5197532ac2f2
I've not used podman and don't fully understand the need for fa123077bc3f39aa0969d883e2d799a054cd4543, so I'm hoping that other people do and can comment on that so I don't need to dive into it. Otherwise, happy to revisit this later.
(https://github.com/bitcoin/bitcoin/pull/27777#pullrequestreview-1451472783)
ACK fa9c65a74cf18e9c75cd3472112d5197532ac2f2
I've not used podman and don't fully understand the need for fa123077bc3f39aa0969d883e2d799a054cd4543, so I'm hoping that other people do and can comment on that so I don't need to dive into it. Otherwise, happy to revisit this later.
💬 stickies-v commented on pull request "ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN":
(https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1210602495)
Okay, I can't comment on that. Since there is no mention of `podman` anywhere else in the codebase/documentation, I think this may make maintaining the CI more opaque, but perhaps that's an okay tradeoff.
(https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1210602495)
Okay, I can't comment on that. Since there is no mention of `podman` anywhere else in the codebase/documentation, I think this may make maintaining the CI more opaque, but perhaps that's an okay tradeoff.
💬 MarcoFalke commented on pull request "ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN":
(https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1210608472)
Yeah, I don't expect anyone to use `RESTART_CI_DOCKER_BEFORE_RUN` beside the persistent workers, so it is an undocumented setting. Anyone using it is expected to infer the documentation from the two lines of code.
(https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1210608472)
Yeah, I don't expect anyone to use `RESTART_CI_DOCKER_BEFORE_RUN` beside the persistent workers, so it is an undocumented setting. Anyone using it is expected to infer the documentation from the two lines of code.
💬 hebasto commented on pull request "Add public Boost headers explicitly":
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210610907)
```
$ git grep -l boost::multi_index::tag
src/node/miner.h
src/txmempool.h
src/txrequest.cpp
```
The symbol is used in the header. IWYU agree :)
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210610907)
```
$ git grep -l boost::multi_index::tag
src/node/miner.h
src/txmempool.h
src/txrequest.cpp
```
The symbol is used in the header. IWYU agree :)
💬 MarcoFalke commented on pull request "Add public Boost headers explicitly":
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210612717)
what is the point in adding it and using it over just `assert` or `Assert` or `BOOST_REQUIRE`?
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210612717)
what is the point in adding it and using it over just `assert` or `Assert` or `BOOST_REQUIRE`?
💬 hebasto commented on pull request "Add public Boost headers explicitly":
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210613769)
I see no point at all. Is it OK to put this change into this PR ?
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210613769)
I see no point at all. Is it OK to put this change into this PR ?
💬 hebasto commented on pull request "Add public Boost headers explicitly":
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210614349)
Or I can just revert the recent change?
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210614349)
Or I can just revert the recent change?
💬 MarcoFalke commented on pull request "Add public Boost headers explicitly":
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210615011)
I meant moving everything to the cpp file, but they are included via txmempool.h either way, so while everything can be moved to the cpp file, it doesn't matter.
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1210615011)
I meant moving everything to the cpp file, but they are included via txmempool.h either way, so while everything can be moved to the cpp file, it doesn't matter.
💬 erikarvstedt commented on issue "bitcoind hangs waiting for `g_requests.empty()`":
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1568888056)
A similar bug appears in the [nix-bitcoin](https://github.com/fort-nix/nix-bitcoin/) [test suite](https://github.com/fort-nix/nix-bitcoin/blob/master/test/tests.py).
The test is executed inside a QEMU VM and runs `bitcoind` together with a bunch of client services (like `clightning`, `lnd`, `joinmarket`). It then stops all services, including `bitcoind`.
Sometimes `bitcoind` hangs during shutdown:
1. First, it waits for a few minutes at [httpserver.cpp#L469](https://github.com/bitcoin/bitco
...
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1568888056)
A similar bug appears in the [nix-bitcoin](https://github.com/fort-nix/nix-bitcoin/) [test suite](https://github.com/fort-nix/nix-bitcoin/blob/master/test/tests.py).
The test is executed inside a QEMU VM and runs `bitcoind` together with a bunch of client services (like `clightning`, `lnd`, `joinmarket`). It then stops all services, including `bitcoind`.
Sometimes `bitcoind` hangs during shutdown:
1. First, it waits for a few minutes at [httpserver.cpp#L469](https://github.com/bitcoin/bitco
...
📝 mzumsande opened a pull request: "test: fix intermittent error in getblockfrompeer.py"
(https://github.com/bitcoin/bitcoin/pull/27784)
This adds an additional `sync_blocks` call, fixing an intermittent error caused by blocks arriving out of order due to how compact block relay may revert to headers processing when the tip hasn't caught up, and resulting in slightly different pruning behavior.
Making sure that all blocks from the previous tests are synced before generating more blocks makes this impossible.
See https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1566354933 and https://github.com/bitcoin/bitcoin/iss
...
(https://github.com/bitcoin/bitcoin/pull/27784)
This adds an additional `sync_blocks` call, fixing an intermittent error caused by blocks arriving out of order due to how compact block relay may revert to headers processing when the tip hasn't caught up, and resulting in slightly different pruning behavior.
Making sure that all blocks from the previous tests are synced before generating more blocks makes this impossible.
See https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1566354933 and https://github.com/bitcoin/bitcoin/iss
...
💬 mzumsande commented on issue "rpc_getblockfrompeer.py intermittent failure: assert_equal(pruneheight, 248); not(249 == 248)":
(https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1568894113)
Opened #27784 which just adds an additional sync, as suggested in https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1567870784 .
(https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1568894113)
Opened #27784 which just adds an additional sync, as suggested in https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1567870784 .
💬 brunoerg commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1210698732)
In 30c39c1316a7b5c1914654f2a1487309b6e550ac: If we're in IBD, wouldn't we put that peer in the end of the queue with no need (I mean, without doing a rec request in fact)?
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1210698732)
In 30c39c1316a7b5c1914654f2a1487309b6e550ac: If we're in IBD, wouldn't we put that peer in the end of the queue with no need (I mean, without doing a rec request in fact)?
💬 Crypt-iQ commented on issue "bitcoind hangs waiting for `g_requests.empty()`":
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1568965050)
@erikarvstedt it seems like there's a call to `importmulti` that is outstanding while bitcoind is trying to shut down
(https://github.com/bitcoin/bitcoin/issues/27722#issuecomment-1568965050)
@erikarvstedt it seems like there's a call to `importmulti` that is outstanding while bitcoind is trying to shut down
💬 aureleoules commented on pull request "ci: Add test coverage job":
(https://github.com/bitcoin/bitcoin/pull/27547#discussion_r1210750673)
Done thanks. I rolled back to jammy.
(https://github.com/bitcoin/bitcoin/pull/27547#discussion_r1210750673)
Done thanks. I rolled back to jammy.
💬 aureleoules commented on pull request "ci: Add test coverage job":
(https://github.com/bitcoin/bitcoin/pull/27547#issuecomment-1568997159)
> I presume there is a setting to disable the comment and just have DrahtBot add a link in the form of https://app.codecov.io/gh/{owner}/{repo}/pull/{number} to the summary comment?
Yes there is. Done in 320eb03aeb097f6d14757c66a1b0bf40d0473b88.
(https://github.com/bitcoin/bitcoin/pull/27547#issuecomment-1568997159)
> I presume there is a setting to disable the comment and just have DrahtBot add a link in the form of https://app.codecov.io/gh/{owner}/{repo}/pull/{number} to the summary comment?
Yes there is. Done in 320eb03aeb097f6d14757c66a1b0bf40d0473b88.
👍 theStack approved a pull request: "test: fix intermittent error in getblockfrompeer.py"
(https://github.com/bitcoin/bitcoin/pull/27784#pullrequestreview-1451720045)
ACK 9fe9074266c6d7ddb40384d81741df1fc94980ff
Unfortunately there seems to be no reliable way to reproduce the issue on master (which could then be also ran on this PR to ensure that it is indeed fixed), but the explanation in https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1566554075 makes sense to me.
(https://github.com/bitcoin/bitcoin/pull/27784#pullrequestreview-1451720045)
ACK 9fe9074266c6d7ddb40384d81741df1fc94980ff
Unfortunately there seems to be no reliable way to reproduce the issue on master (which could then be also ran on this PR to ensure that it is indeed fixed), but the explanation in https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1566554075 makes sense to me.
💬 pinheadmz commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#issuecomment-1569013910)
> 🐙 This pull request conflicts with the target branch and [needs rebase](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes).
Relax bot! sheesh. I'm gonna wait until #27375 gets approved
(https://github.com/bitcoin/bitcoin/pull/27679#issuecomment-1569013910)
> 🐙 This pull request conflicts with the target branch and [needs rebase](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes).
Relax bot! sheesh. I'm gonna wait until #27375 gets approved
💬 Sjors commented on pull request "test: fix intermittent error in getblockfrompeer.py":
(https://github.com/bitcoin/bitcoin/pull/27784#issuecomment-1569023147)
9fe9074266c6d7ddb40384d81741df1fc94980ff looks harmless, but I haven't delved into the underlying issue. Can the original be reproduced by running the test in a loop??
CI failure seems spurious.
(https://github.com/bitcoin/bitcoin/pull/27784#issuecomment-1569023147)
9fe9074266c6d7ddb40384d81741df1fc94980ff looks harmless, but I haven't delved into the underlying issue. Can the original be reproduced by running the test in a loop??
CI failure seems spurious.
💬 theStack commented on pull request "Fix rpc_getblockfrompeer intermittency by introducing 'getblockfileinfo' RPC command":
(https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1569050370)
I agree with @furszy and @mzumsande that a `getblockfileinfo` RPC could be quite handy, both for making some of the pruning-related tests more robust and readable (potentially eliminating some magic numbers that can only be explained by the one who introduced them) and also for testing AssumeUTXO. As an illustration, the blocks directory of a mainnet test-run of AssumeUTXO ended up at some point like this on my disk:
```
$ ls -l ./datadir2/blocks/blk*.dat
-rw------- 1 thestack thestack 13
...
(https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1569050370)
I agree with @furszy and @mzumsande that a `getblockfileinfo` RPC could be quite handy, both for making some of the pruning-related tests more robust and readable (potentially eliminating some magic numbers that can only be explained by the one who introduced them) and also for testing AssumeUTXO. As an illustration, the blocks directory of a mainnet test-run of AssumeUTXO ended up at some point like this on my disk:
```
$ ls -l ./datadir2/blocks/blk*.dat
-rw------- 1 thestack thestack 13
...