💬 maflcko commented on issue "ERROR: AcceptBlockHeader: prev block not found":
(https://github.com/bitcoin/bitcoin/issues/31905#issuecomment-2668974005)
Your are running an EOL version of Bitcoin Core.
Is this still an issue with a recent version of Bitcoin Core?
(https://github.com/bitcoin/bitcoin/issues/31905#issuecomment-2668974005)
Your are running an EOL version of Bitcoin Core.
Is this still an issue with a recent version of Bitcoin Core?
💬 glozow commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1961889923)
Another option would be to revert this.
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1961889923)
Another option would be to revert this.
💬 yancyribbens commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#discussion_r1961892017)
I agree the TODO isn't very clear and possibly no longer applicable. Maybe it could be rounded up with other unclear comments and TODOs in a separate PR. A PR to just remove this one TODO line seems silly.
(https://github.com/bitcoin/bitcoin/pull/31896#discussion_r1961892017)
I agree the TODO isn't very clear and possibly no longer applicable. Maybe it could be rounded up with other unclear comments and TODOs in a separate PR. A PR to just remove this one TODO line seems silly.
🤔 ryanofsky reviewed a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2627160751)
Updated 66c318abe1b7df3e2ff1035376bc5f4b2059626a -> 828c440b0dea29ab41ffc32d88969176d1286655 ([`pr/subtree.17`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.17) -> [`pr/subtree.18`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.18), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/subtree.17..pr/subtree.18)) fixing cmake whitespace.
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2627160751)
Updated 66c318abe1b7df3e2ff1035376bc5f4b2059626a -> 828c440b0dea29ab41ffc32d88969176d1286655 ([`pr/subtree.17`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.17) -> [`pr/subtree.18`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.18), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/subtree.17..pr/subtree.18)) fixing cmake whitespace.
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1961881829)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1961422233
Good catch, fixed indentation and spacing. I don't think it should be too much of a burden to enforce a consistent style for cmake code given how little cmake code there is relatively. There also seems to be a [cmake-lint](https://cmake-format.readthedocs.io/en/latest/lint-usage.html) tool that could maybe help.
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1961881829)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1961422233
Good catch, fixed indentation and spacing. I don't think it should be too much of a burden to enforce a consistent style for cmake code given how little cmake code there is relatively. There also seems to be a [cmake-lint](https://cmake-format.readthedocs.io/en/latest/lint-usage.html) tool that could maybe help.
💬 hodlinator commented on issue "qa: timeout in StopHTTPServer()":
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2668999318)
I'm working on making the code a bit more robust:
- Impose a timeout on HTTP connections shutting down - Maybe related to this issue.
- Fix an issue where we were not always processing the event to free eventHTTP - Would probably just leak otherwise, shutdown completes regardless of this in my testing.
```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 88e640c377..ce2534fbd0 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -197,10 +197,15 @@ public:
return WIT
...
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2668999318)
I'm working on making the code a bit more robust:
- Impose a timeout on HTTP connections shutting down - Maybe related to this issue.
- Fix an issue where we were not always processing the event to free eventHTTP - Would probably just leak otherwise, shutdown completes regardless of this in my testing.
```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 88e640c377..ce2534fbd0 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -197,10 +197,15 @@ public:
return WIT
...
💬 Sjors commented on pull request "rpc: add optional blockhash to waitfornewblock, unhide in help":
(https://github.com/bitcoin/bitcoin/pull/30635#issuecomment-2668999401)
This is now based on #31785, and we now unhide all three wait methods.
(https://github.com/bitcoin/bitcoin/pull/30635#issuecomment-2668999401)
This is now based on #31785, and we now unhide all three wait methods.
📝 darosior opened a pull request: "Revert merge of PR #31826"
(https://github.com/bitcoin/bitcoin/pull/31908)
PR #31826 was merged [despite the code not compiling](https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1961315638).
#31902 was opened to fix the code but since this code is only targeting a not officially supported platform, we don't have a CI in place to compile and run tests on this platform, neither apparently reviewers do (nor does the author?), don't take more risk right before 29 and revert the original broken PR.
(https://github.com/bitcoin/bitcoin/pull/31908)
PR #31826 was merged [despite the code not compiling](https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1961315638).
#31902 was opened to fix the code but since this code is only targeting a not officially supported platform, we don't have a CI in place to compile and run tests on this platform, neither apparently reviewers do (nor does the author?), don't take more risk right before 29 and revert the original broken PR.
👍 pablomartin4btc approved a pull request: "cmake: Exclude generated sources from translation"
(https://github.com/bitcoin/bitcoin/pull/31899#pullrequestreview-2627202385)
tACK ff4ddd3d2e3b08156fd0a0aeb954fde0a5f4cb03
Tested in both Ubuntu and macOS.
(https://github.com/bitcoin/bitcoin/pull/31899#pullrequestreview-2627202385)
tACK ff4ddd3d2e3b08156fd0a0aeb954fde0a5f4cb03
Tested in both Ubuntu and macOS.
💬 mzumsande commented on issue "wallet: wrong balance and crash after reorg and unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/31824#issuecomment-2669011985)
> Is this not where the chainstate related changes are flushed to disk?
> https://github.com/bitcoin/bitcoin/blob/28.x/src/validation.cpp#L3069
The call uses `FlushStateMode::IF_NEEDED`, which means it usually won't do anything - it will only flush if it's necessary because the cache has grown to a critical size.
(https://github.com/bitcoin/bitcoin/issues/31824#issuecomment-2669011985)
> Is this not where the chainstate related changes are flushed to disk?
> https://github.com/bitcoin/bitcoin/blob/28.x/src/validation.cpp#L3069
The call uses `FlushStateMode::IF_NEEDED`, which means it usually won't do anything - it will only flush if it's necessary because the cache has grown to a critical size.
💬 darosior commented on pull request "random: move VerifyRNDRRS above InitHardwareRand":
(https://github.com/bitcoin/bitcoin/pull/31902#issuecomment-2669021440)
I think for now it's safer to just revert the original PR https://github.com/bitcoin/bitcoin/pull/31908. The changes can be re-considered and re-reviewed after the 29.0 branch-off.
(https://github.com/bitcoin/bitcoin/pull/31902#issuecomment-2669021440)
I think for now it's safer to just revert the original PR https://github.com/bitcoin/bitcoin/pull/31908. The changes can be re-considered and re-reviewed after the 29.0 branch-off.
✅ tnull closed a pull request: "Fix field name styling in `submitpackage` RPC results"
(https://github.com/bitcoin/bitcoin/pull/31900)
(https://github.com/bitcoin/bitcoin/pull/31900)
💬 tnull commented on pull request "Fix field name styling in `submitpackage` RPC results":
(https://github.com/bitcoin/bitcoin/pull/31900#issuecomment-2669032754)
> Ok... even though it's marked as experimental, I think it's too late to break `submitpackage` purely for style reasons. Fwiw I did think this was worthwhile before (and I pinged @tnull after seeing [comment](https://github.com/spesmilo/electrum-protocol/issues/4#issuecomment-2665116596)), but I'm leaning towards nack now. My apologies!
Alright, no worries! Going ahead and closing this.
(https://github.com/bitcoin/bitcoin/pull/31900#issuecomment-2669032754)
> Ok... even though it's marked as experimental, I think it's too late to break `submitpackage` purely for style reasons. Fwiw I did think this was worthwhile before (and I pinged @tnull after seeing [comment](https://github.com/spesmilo/electrum-protocol/issues/4#issuecomment-2665116596)), but I'm leaning towards nack now. My apologies!
Alright, no worries! Going ahead and closing this.
💬 glozow commented on pull request "Revert merge of PR #31826":
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2669053075)
Up to reviewers to decide whether this or #31902 is more appropriate, but added to milestone so we do this before branch off.
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2669053075)
Up to reviewers to decide whether this or #31902 is more appropriate, but added to milestone so we do this before branch off.
💬 maflcko commented on issue "Intermittent issue in p2p_i2p_ports.py AssertionError: [node 0] Expected messages "['Error connecting to [...].b32.i2p:0: Cannot connect to 127.0.0.1:60000']" does not partially match log:":
(https://github.com/bitcoin/bitcoin/issues/30030#issuecomment-2669069081)
So the issue is internal to the functional tests themselves. See https://cirrus-ci.com/task/5346278168592384?logs=ci#L4358 (commit https://github.com/bitcoin/bitcoin/commit/fa780b31ab54febd7e56a0fc165683e056fc2500) which prints:
```
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
python3 50589 root 11u IPv4 179599797 0t0 TCP localhost:60000->localhost:19216 (ESTABLISHED)
bitcoind 50594 root 21u IPv4 179704903 0t0 TCP localhost:19216->localhost:60000 (ESTABLISHED)
...
(https://github.com/bitcoin/bitcoin/issues/30030#issuecomment-2669069081)
So the issue is internal to the functional tests themselves. See https://cirrus-ci.com/task/5346278168592384?logs=ci#L4358 (commit https://github.com/bitcoin/bitcoin/commit/fa780b31ab54febd7e56a0fc165683e056fc2500) which prints:
```
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
python3 50589 root 11u IPv4 179599797 0t0 TCP localhost:60000->localhost:19216 (ESTABLISHED)
bitcoind 50594 root 21u IPv4 179704903 0t0 TCP localhost:19216->localhost:60000 (ESTABLISHED)
...
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1961955650)
`clang-format` doesn't seem to touch this. However once I added some line breaks myself, it suddenly had an opinion. So I'll commit that.
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1961955650)
`clang-format` doesn't seem to touch this. However once I added some line breaks myself, it suddenly had an opinion. So I'll commit that.
💬 0xB10C commented on pull request "ci: Switch to gcr.io mirror to avoid rate limits":
(https://github.com/bitcoin/bitcoin/pull/31906#issuecomment-2669078279)
An alternative would probably be to configure the docker deamons of the self-hosted runners to use the mirror?
in e.g. `/etc/docker/daemon.json`
```
{
"registry-mirrors": ["https://mirror.gcr.io"]
}
```
or in `/etc/containers/registries.conf.d/gcr.conf` for podman
```
[[registry]]
location = "https://mirror.gcr.io"
prefix = "docker.io"
```
(https://github.com/bitcoin/bitcoin/pull/31906#issuecomment-2669078279)
An alternative would probably be to configure the docker deamons of the self-hosted runners to use the mirror?
in e.g. `/etc/docker/daemon.json`
```
{
"registry-mirrors": ["https://mirror.gcr.io"]
}
```
or in `/etc/containers/registries.conf.d/gcr.conf` for podman
```
[[registry]]
location = "https://mirror.gcr.io"
prefix = "docker.io"
```
💬 sipa commented on pull request "Revert merge of PR #31826":
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2669082332)
ACK 3e9b12b3e0f039a8760410afed74c7e4d15afbe6
We shouldn't have merged #31826 if it didn't compile, and apparently nobody actually tested it. It's one thing to make changes for an unsupported platform that don't hurt others, but clearly the bar for testing was too low if it actually doesn't compile at all.
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2669082332)
ACK 3e9b12b3e0f039a8760410afed74c7e4d15afbe6
We shouldn't have merged #31826 if it didn't compile, and apparently nobody actually tested it. It's one thing to make changes for an unsupported platform that don't hurt others, but clearly the bar for testing was too low if it actually doesn't compile at all.
💬 0xB10C commented on pull request "ci: Switch to gcr.io mirror to avoid rate limits":
(https://github.com/bitcoin/bitcoin/pull/31906#discussion_r1961960848)
the ASAN job (and others) currently runs on a GitHub hosted runner and these are exempt from the rate-limiting: https://github.com/actions/runner-images/issues/1445#issuecomment-714832439
(https://github.com/bitcoin/bitcoin/pull/31906#discussion_r1961960848)
the ASAN job (and others) currently runs on a GitHub hosted runner and these are exempt from the rate-limiting: https://github.com/actions/runner-images/issues/1445#issuecomment-714832439
💬 dergoegge commented on pull request "Revert merge of PR #31826":
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2669090900)
> neither apparently reviewers do (nor does the author?)
It was actually tested on the merged PR: see https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2652823871 and https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2653350018 but a later refactoring suggestion (https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1955192539) introduced the compilation issue and no re-testing was done before merge.
I think if @laanwj and @giahuy98 are willing to re-test on #31902 we
...
(https://github.com/bitcoin/bitcoin/pull/31908#issuecomment-2669090900)
> neither apparently reviewers do (nor does the author?)
It was actually tested on the merged PR: see https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2652823871 and https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2653350018 but a later refactoring suggestion (https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1955192539) introduced the compilation issue and no re-testing was done before merge.
I think if @laanwj and @giahuy98 are willing to re-test on #31902 we
...