📝 fanquake opened a pull request: "guix: fix CURL disable flag in osslsigncode"
(https://github.com/bitcoin/bitcoin/pull/27779)
See https://cmake.org/cmake/help/latest/module/FindCURL.html.
(https://github.com/bitcoin/bitcoin/pull/27779)
See https://cmake.org/cmake/help/latest/module/FindCURL.html.
💬 MarcoFalke commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1568242621)
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=59285
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1568242621)
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=59285
💬 hebasto commented on pull request "guix: fix CURL disable flag in osslsigncode":
(https://github.com/bitcoin/bitcoin/pull/27779#issuecomment-1568258065)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/27779#issuecomment-1568258065)
Concept ACK.
💬 stickies-v commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1210171462)
We don't need `m_uri` to be a valid URI, that's what `evhttp_uri_parse` is checking, we just need it to not behave unexpectedly (eg. segfault) when passed to `evhttp_uri_parse`. "a" also would not be a valid URI, but we're not checking that here either, that would just be duplicating what `evhttp_uri_parse` is already doing.
So unless there's another reason to not pass an empty char array to `evhttp_uri_parse`, I'd leave it out yeah.
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1210171462)
We don't need `m_uri` to be a valid URI, that's what `evhttp_uri_parse` is checking, we just need it to not behave unexpectedly (eg. segfault) when passed to `evhttp_uri_parse`. "a" also would not be a valid URI, but we're not checking that here either, that would just be duplicating what `evhttp_uri_parse` is already doing.
So unless there's another reason to not pass an empty char array to `evhttp_uri_parse`, I'd leave it out yeah.
💬 hebasto commented on pull request "guix: fix CURL disable flag in osslsigncode":
(https://github.com/bitcoin/bitcoin/pull/27779#issuecomment-1568322163)
It is not clear why this is a fix. `CMAKE_DISABLE_FIND_PACKAGE_CURL` is a valid [variable](https://cmake.org/cmake/help/latest/variable/CMAKE_DISABLE_FIND_PACKAGE_PackageName.html) that:
> can be used to build a project without an optional package
(https://github.com/bitcoin/bitcoin/pull/27779#issuecomment-1568322163)
It is not clear why this is a fix. `CMAKE_DISABLE_FIND_PACKAGE_CURL` is a valid [variable](https://cmake.org/cmake/help/latest/variable/CMAKE_DISABLE_FIND_PACKAGE_PackageName.html) that:
> can be used to build a project without an optional package
🤔 stickies-v reviewed a pull request: "httpserver, rest: improving URI validation"
(https://github.com/bitcoin/bitcoin/pull/27253#pullrequestreview-1450797669)
I'm in favour of doing the `replySent` refactoring in a separate commit, but it makes more sense to have that be the first commit imo, to avoid adding logic and then removing it in the very next commit.
(https://github.com/bitcoin/bitcoin/pull/27253#pullrequestreview-1450797669)
I'm in favour of doing the `replySent` refactoring in a separate commit, but it makes more sense to have that be the first commit imo, to avoid adding logic and then removing it in the very next commit.
💬 stickies-v commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1210184105)
Don't we need to clean this up regardless of whether an exception was thrown?
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1210184105)
Don't we need to clean this up regardless of whether an exception was thrown?
📝 MarcoFalke opened a pull request: "fuzz: Avoid timeout in utxo_total_supply"
(https://github.com/bitcoin/bitcoin/pull/27780)
Looks like for high block counts it may be better to mock the chain, otherwise a high limit will lead to fuzz input bloat and timeouts, see https://github.com/bitcoin/bitcoin/pull/17860#issuecomment-1538252773.
It can be checked that the fuzz target can still find the CVE, see https://github.com/bitcoin/bitcoin/pull/17860#pullrequestreview-1410594057 with a diff of:
```diff
diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp
index f949655909..6f4cfb5f51 100644
--- a/src
...
(https://github.com/bitcoin/bitcoin/pull/27780)
Looks like for high block counts it may be better to mock the chain, otherwise a high limit will lead to fuzz input bloat and timeouts, see https://github.com/bitcoin/bitcoin/pull/17860#issuecomment-1538252773.
It can be checked that the fuzz target can still find the CVE, see https://github.com/bitcoin/bitcoin/pull/17860#pullrequestreview-1410594057 with a diff of:
```diff
diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp
index f949655909..6f4cfb5f51 100644
--- a/src
...
💬 furszy commented on pull request "Fix rpc_getblockfrompeer intermittency by introducing 'getblockfileinfo' RPC command":
(https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1568357831)
> What about a one-line fix to clear the test issue and then a follow-up with the RPC, which may take longer to review than a trivial one-line patch?
np for me. @mzumsande do you want to go ahead with your fix? I can just relabel this PR to "Implement getblockfileinfo RPC command" and done.
(https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-1568357831)
> What about a one-line fix to clear the test issue and then a follow-up with the RPC, which may take longer to review than a trivial one-line patch?
np for me. @mzumsande do you want to go ahead with your fix? I can just relabel this PR to "Implement getblockfileinfo RPC command" and done.
💬 stickies-v commented on pull request "ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN":
(https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1210209382)
Would it be prudent to use the [`--filter` option](https://docs.docker.com/engine/reference/commandline/image_prune/#filter) here to only prune images created by the CI? This may be especially helpful to people running the CI locally/unsandboxed. We'd just need to add a `--label "bitcoin-ci"` argument to `docker build`.
```suggestion
docker image prune --force --filter label=bitcoin-ci
```
(https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1210209382)
Would it be prudent to use the [`--filter` option](https://docs.docker.com/engine/reference/commandline/image_prune/#filter) here to only prune images created by the CI? This may be especially helpful to people running the CI locally/unsandboxed. We'd just need to add a `--label "bitcoin-ci"` argument to `docker build`.
```suggestion
docker image prune --force --filter label=bitcoin-ci
```
💬 alexanderwiederin commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1210211360)
Getting `syntax error: operand expected (error token is ""30000" + "20000"")` on macOS
```suggestion
FINAL_HEIGHT=$(($BASE_HEIGHT + $INCREMENTAL_HEIGHT))
```
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1210211360)
Getting `syntax error: operand expected (error token is ""30000" + "20000"")` on macOS
```suggestion
FINAL_HEIGHT=$(($BASE_HEIGHT + $INCREMENTAL_HEIGHT))
```
💬 alexanderwiederin commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1210212400)
Missing the second "t" of "Starting"
```suggestion
echo "-- Starting the server node to provide blocks to the client node..."
```
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1210212400)
Missing the second "t" of "Starting"
```suggestion
echo "-- Starting the server node to provide blocks to the client node..."
```
💬 hebasto commented on pull request "guix: fix CURL disable flag in osslsigncode":
(https://github.com/bitcoin/bitcoin/pull/27779#issuecomment-1568365679)
Btw, do we still need this line https://github.com/bitcoin/bitcoin/blob/f467b28ac35add304442f30c2a05ef5d9df496e2/contrib/guix/manifest.scm#L14 at all?
(https://github.com/bitcoin/bitcoin/pull/27779#issuecomment-1568365679)
Btw, do we still need this line https://github.com/bitcoin/bitcoin/blob/f467b28ac35add304442f30c2a05ef5d9df496e2/contrib/guix/manifest.scm#L14 at all?
💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1568366234)
> I'm in favour of doing the `replySent` refactoring in a separate commit, but it makes more sense to have that be the first commit imo, to avoid adding logic and then removing it in the very next commit.
Yeah, true, I agree... let me revert the order of the commits.
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1568366234)
> I'm in favour of doing the `replySent` refactoring in a separate commit, but it makes more sense to have that be the first commit imo, to avoid adding logic and then removing it in the very next commit.
Yeah, true, I agree... let me revert the order of the commits.
💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1210218266)
Let me check again when I do revert the order of the commits, when I tested before those were needed as the fuzz was complaining about memory leaks.
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1210218266)
Let me check again when I do revert the order of the commits, when I tested before those were needed as the fuzz was complaining about memory leaks.
💬 willcl-ark commented on issue "Libbitcoinkernel Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27587#issuecomment-1568419077)
Would it make sense to update the project page to https://github.com/orgs/bitcoin/projects/3/views/1 (which if I'm not mistaken seems to be the current one)?
Could keep a link to the old project https://github.com/bitcoin/bitcoin/projects/18 for reference?
(https://github.com/bitcoin/bitcoin/issues/27587#issuecomment-1568419077)
Would it make sense to update the project page to https://github.com/orgs/bitcoin/projects/3/views/1 (which if I'm not mistaken seems to be the current one)?
Could keep a link to the old project https://github.com/bitcoin/bitcoin/projects/18 for reference?
💬 Sjors commented on issue "Spurious (?) valgrind failure for p2p_compactblocks.py":
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1568421101)
I made a note to try with clang instead.
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1568421101)
I made a note to try with clang instead.
💬 Sjors commented on issue "rpc_getblockfrompeer.py intermittent failure: assert_equal(pruneheight, 248); not(249 == 248)":
(https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1568429590)
cc @fjahr since he worked on the prune part of this test.
(https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1568429590)
cc @fjahr since he worked on the prune part of this test.
👍 furszy approved a pull request: "wallet, bench: Move commonly used functions to their own file and fix a bug"
(https://github.com/bitcoin/bitcoin/pull/27666#pullrequestreview-1450932691)
ACK c61d3f02
(https://github.com/bitcoin/bitcoin/pull/27666#pullrequestreview-1450932691)
ACK c61d3f02
💬 furszy commented on pull request "wallet, bench: Move commonly used functions to their own file and fix a bug":
(https://github.com/bitcoin/bitcoin/pull/27666#discussion_r1210268191)
Not for this PR as it touches the wallet internals but the duplicated `AddToWallet` calls shouldn't be needed if the wallet wtx state is equal to the one in the event.
(https://github.com/bitcoin/bitcoin/pull/27666#discussion_r1210268191)
Not for this PR as it touches the wallet internals but the duplicated `AddToWallet` calls shouldn't be needed if the wallet wtx state is equal to the one in the event.