💬 instagibbs commented on pull request "p2p: When close to the tip, download blocks in parallel from additional peers to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/29664#discussion_r1768742804)
`MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK` seems like a variable we could rename to be more general and use here, especially if both mechanisms could fire
(https://github.com/bitcoin/bitcoin/pull/29664#discussion_r1768742804)
`MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK` seems like a variable we could rename to be more general and use here, especially if both mechanisms could fire
💬 bitcoin-tools commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363889298)
> worked around by installing python3
That explains why it doesn't run on certain docker base images. Is python3 considered a runtime dependency for test_bitcoin?
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363889298)
> worked around by installing python3
That explains why it doesn't run on certain docker base images. Is python3 considered a runtime dependency for test_bitcoin?
💬 fanquake commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363889851)
I think the bug is that commit introduced a (silent) runtime dependency that isn't actually checked/enforced in anyway.
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363889851)
I think the bug is that commit introduced a (silent) runtime dependency that isn't actually checked/enforced in anyway.
💬 maflcko commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363895917)
Yeah, python3 is needed for ctest, IIUC. However, test_bitcoin is probably considered stand-alone and shouldn't depend on anything other than itself.
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363895917)
Yeah, python3 is needed for ctest, IIUC. However, test_bitcoin is probably considered stand-alone and shouldn't depend on anything other than itself.
💬 fanquake commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363896359)
Yea. Even if `ctest`/development likely requires it, the `test_bitcoin` release binary we are shipping to end users shouldn't have a (undocumented) dependency on invoking Python.
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363896359)
Yea. Even if `ctest`/development likely requires it, the `test_bitcoin` release binary we are shipping to end users shouldn't have a (undocumented) dependency on invoking Python.
💬 maflcko commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363906135)
The cleanest fix would probably be https://github.com/bitcoin/bitcoin/pull/29868/files#diff-4ea52ba9a5642d6946fb0016ca168b6647a8c6403d9e15e2a7197e72022824d4R87 `const std::string command{"sh -c 'echo err 1>&2 && false'"};`.
Previously `ls` was a requirement, so making it `sh` seems fine as well?
cc @hebasto
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2363906135)
The cleanest fix would probably be https://github.com/bitcoin/bitcoin/pull/29868/files#diff-4ea52ba9a5642d6946fb0016ca168b6647a8c6403d9e15e2a7197e72022824d4R87 `const std::string command{"sh -c 'echo err 1>&2 && false'"};`.
Previously `ls` was a requirement, so making it `sh` seems fine as well?
cc @hebasto
💬 maflcko commented on pull request "fuzz: Add check in `p2p_headers_presync` that chain work never exceeds minimum work":
(https://github.com/bitcoin/bitcoin/pull/30918#issuecomment-2363907360)
review ACK 284bd17309ab3b124d9dcddfec62f5506383343b
(https://github.com/bitcoin/bitcoin/pull/30918#issuecomment-2363907360)
review ACK 284bd17309ab3b124d9dcddfec62f5506383343b
💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1768765758)
I think this line got pulled out of the `else if` conditional. Won't this set the value for all unique parents and reject any transaction with two unique prevout hashes?
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1768765758)
I think this line got pulled out of the `else if` conditional. Won't this set the value for all unique parents and reject any transaction with two unique prevout hashes?
✅ fanquake closed a pull request: "depends: Qt 5.15.15"
(https://github.com/bitcoin/bitcoin/pull/30774)
(https://github.com/bitcoin/bitcoin/pull/30774)
🚀 fanquake merged a pull request: "refactor: move `SignSignature` helpers to test utils"
(https://github.com/bitcoin/bitcoin/pull/30561)
(https://github.com/bitcoin/bitcoin/pull/30561)
💬 l0rinc commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768725247)
I'm not sure I understand why we need this.
Why not make them all runtime instead, why are we "testing" compile time behavior here at all, aren't we already doing that in the rest of the source code?
It seems to me the whole situation would simplify a lot if we would only test runtime behavior here.
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768725247)
I'm not sure I understand why we need this.
Why not make them all runtime instead, why are we "testing" compile time behavior here at all, aren't we already doing that in the rest of the source code?
It seems to me the whole situation would simplify a lot if we would only test runtime behavior here.
💬 l0rinc commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768784651)
I like the idea of testing the validator together with the implementation!
But I'm not in love with the current approach.
Why are we testing `PassFmt<1>("%02d");` with `foo` - I don't think it helps with understanding how formatter works.
Also `PassFmt<1>("%s")` made sense when we were only validating the number of args, but we've extended it since, I think we should extend examples with the actual parameters, e.g.
* `PassFmt("%s", "test");`
* `PassFmt("%02d", 42);`
* `PassFmt("%12$s
...
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768784651)
I like the idea of testing the validator together with the implementation!
But I'm not in love with the current approach.
Why are we testing `PassFmt<1>("%02d");` with `foo` - I don't think it helps with understanding how formatter works.
Also `PassFmt<1>("%s")` made sense when we were only validating the number of args, but we've extended it since, I think we should extend examples with the actual parameters, e.g.
* `PassFmt("%s", "test");`
* `PassFmt("%02d", 42);`
* `PassFmt("%12$s
...
💬 tdb3 commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1768746217)
nit: If touching the file again, looks like this could be simplified.
```diff
- bool IsVersionSelected() const { return m_details_level == 3 || m_details_level >= 4; }
+ bool IsVersionSelected() const { return m_details_level >= 3; }
```
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1768746217)
nit: If touching the file again, looks like this could be simplified.
```diff
- bool IsVersionSelected() const { return m_details_level == 3 || m_details_level >= 4; }
+ bool IsVersionSelected() const { return m_details_level >= 3; }
```
🤔 tdb3 reviewed a pull request: "netinfo: add peer services column and outbound-only peers list"
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2318488868)
Left a couple comments.
Tested that using `5` does include only outbound peers by connecting two nodes locally.
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2318488868)
Left a couple comments.
Tested that using `5` does include only outbound peers by connecting two nodes locally.
💬 tdb3 commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1768786471)
Do we expect to receive a null in `servicesnames` among other entries?
If we don't, maybe this should be something like `assert(!services[i].isNull());` so the issue can be noticed.
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1768786471)
Do we expect to receive a null in `servicesnames` among other entries?
If we don't, maybe this should be something like `assert(!services[i].isNull());` so the issue can be noticed.
💬 bitcoin-tools commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2364028677)
Can it be simplified to `const std::string command{"echo err 1>&2 && false"};` or is `sh -c '...'` necessary to ensure portability?
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2364028677)
Can it be simplified to `const std::string command{"echo err 1>&2 && false"};` or is `sh -c '...'` necessary to ensure portability?
💬 achow101 commented on pull request "cli: Improve error message on multiwallet cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2364029768)
ACK 54227e681a4efa8961f1ad05d43366d88a9b686a
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2364029768)
ACK 54227e681a4efa8961f1ad05d43366d88a9b686a
💬 maflcko commented on issue "`test_bitcoin` from pre-built 28.0rc2 tarball is failing for JSON parsing":
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2364037617)
I do not know, but I think that `echo` and `false` would fall back to the coreutils one, if the shell built-in isn't available. The previous requirement `ls` is also from coreutils, IIUC, so it should be fine as well.
(https://github.com/bitcoin/bitcoin/issues/30938#issuecomment-2364037617)
I do not know, but I think that `echo` and `false` would fall back to the coreutils one, if the shell built-in isn't available. The previous requirement `ls` is also from coreutils, IIUC, so it should be fine as well.
💬 Sjors commented on pull request "Stratum v2 Transport":
(https://github.com/bitcoin/bitcoin/pull/30315#issuecomment-2364044654)
Each of these CI failures seem spurious, some sort of timeout after many hours. However there's real failures in the followup PR. Going to rebase this to avoid confusion.
(https://github.com/bitcoin/bitcoin/pull/30315#issuecomment-2364044654)
Each of these CI failures seem spurious, some sort of timeout after many hours. However there's real failures in the followup PR. Going to rebase this to avoid confusion.
💬 achow101 commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1768862688)
The relative path is relative to the wallet directory, not the default wallet directory. Starting with `-walletdir=<dir>` will change the wallet directory and specifying a wallet name in this argument will be interpreted to be relative to that walletdir.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1768862688)
The relative path is relative to the wallet directory, not the default wallet directory. Starting with `-walletdir=<dir>` will change the wallet directory and specifying a wallet name in this argument will be interpreted to be relative to that walletdir.