💬 glozow commented on pull request "Fix field name styling in `submitpackage` RPC results":
(https://github.com/bitcoin/bitcoin/pull/31900#issuecomment-2668959139)
> Alright, good to know. It might be noteworthy that there are other projects that directly forward Core's JSON (e.g. the Esplora implementations over at https://github.com/mempool/electrs/pull/105 and https://github.com/Blockstream/electrs/pull/119) that would also be affected by the API breakage
Ah right, I forgot about that. And https://github.com/spesmilo/electrumx/pull/288 and https://github.com/spesmilo/electrum-protocol/pull/1. I think this would inconvenience a few people.
Ok... ev
...
(https://github.com/bitcoin/bitcoin/pull/31900#issuecomment-2668959139)
> Alright, good to know. It might be noteworthy that there are other projects that directly forward Core's JSON (e.g. the Esplora implementations over at https://github.com/mempool/electrs/pull/105 and https://github.com/Blockstream/electrs/pull/119) that would also be affected by the API breakage
Ah right, I forgot about that. And https://github.com/spesmilo/electrumx/pull/288 and https://github.com/spesmilo/electrum-protocol/pull/1. I think this would inconvenience a few people.
Ok... ev
...
💬 l0rinc commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14/C++17)":
(https://github.com/bitcoin/bitcoin/pull/31904#discussion_r1961881831)
Haven't noticed the header, will push a PR to https://github.com/llvm/llvm-project/blob/main/compiler-rt/include/fuzzer/FuzzedDataProvider.h#L206
Should I remove it from here in the meantime?
(https://github.com/bitcoin/bitcoin/pull/31904#discussion_r1961881831)
Haven't noticed the header, will push a PR to https://github.com/llvm/llvm-project/blob/main/compiler-rt/include/fuzzer/FuzzedDataProvider.h#L206
Should I remove it from here in the meantime?
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961882621)
Good catch, that's also consistent with `waitforblock`.
(https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961882621)
Good catch, that's also consistent with `waitforblock`.
✅ maflcko closed an issue: "ERROR: AcceptBlockHeader: prev block not found"
(https://github.com/bitcoin/bitcoin/issues/31905)
(https://github.com/bitcoin/bitcoin/issues/31905)
💬 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.