📝 maflcko opened a pull request: "ci: Add missing python3-dev package for riscv64"
(https://github.com/bitcoin/bitcoin/pull/33746)
This is required to compile the pip wheels on native riscv64.
(https://github.com/bitcoin/bitcoin/pull/33746)
This is required to compile the pip wheels on native riscv64.
💬 maflcko commented on pull request "ci: Add missing python3-dev package for riscv64":
(https://github.com/bitcoin/bitcoin/pull/33746#issuecomment-3467952513)
Otherwise the CI output will look like:
```
+ retry -- pip3 install --break-system-packages pycapnp
Collecting pycapnp
Downloading pycapnp-2.2.1.tar.gz (709 kB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 709.1/709.1 kB 539.2 kB/s eta 0:00:00
Installing build dependencies: started
Installing build dependencies: finished with status 'done'
Getting requirements to build wheel: started
Getting requirements to build wheel: finished with status 'done'
Preparing metadata (pyproj
...
(https://github.com/bitcoin/bitcoin/pull/33746#issuecomment-3467952513)
Otherwise the CI output will look like:
```
+ retry -- pip3 install --break-system-packages pycapnp
Collecting pycapnp
Downloading pycapnp-2.2.1.tar.gz (709 kB)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 709.1/709.1 kB 539.2 kB/s eta 0:00:00
Installing build dependencies: started
Installing build dependencies: finished with status 'done'
Getting requirements to build wheel: started
Getting requirements to build wheel: finished with status 'done'
Preparing metadata (pyproj
...
💬 maflcko commented on pull request "ci: Add missing python3-dev package for riscv64":
(https://github.com/bitcoin/bitcoin/pull/33746#issuecomment-3467963658)
This is based on commit 72511fd02e72b74be11273e97bd7911786a82e54 to avoid the clang-16 ICE, so the CI should now pass on `FILE_ENV="./ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh"`.
(https://github.com/bitcoin/bitcoin/pull/33746#issuecomment-3467963658)
This is based on commit 72511fd02e72b74be11273e97bd7911786a82e54 to avoid the clang-16 ICE, so the CI should now pass on `FILE_ENV="./ci/test/00_setup_env_native_nowallet_libbitcoinkernel.sh"`.
💬 instagibbs commented on pull request "fees: rpc: `estimatesmartfee` now returns a fee rate estimate during low network activity":
(https://github.com/bitcoin/bitcoin/pull/32395#issuecomment-3467999278)
I'm generally a -0 on this. I'd rather energy be spent making it easier to fetch "real" estimates, as per your other work?
(https://github.com/bitcoin/bitcoin/pull/32395#issuecomment-3467999278)
I'm generally a -0 on this. I'd rather energy be spent making it easier to fetch "real" estimates, as per your other work?
💬 purpleKarrot commented on pull request "fuzz: refactor memcpy to std::ranges::copy to work around ubsan warn":
(https://github.com/bitcoin/bitcoin/pull/33743#issuecomment-3468011813)
> * It works around an UB-Sanitizer bug, when the source range is empty.
I guess what you want to express is:
* It has defined semantics for an empty source ranges.
Passing a zero length to `memcpy` is UB per the language standard. This is not an "UB-Sanitizer bug".
(https://github.com/bitcoin/bitcoin/pull/33743#issuecomment-3468011813)
> * It works around an UB-Sanitizer bug, when the source range is empty.
I guess what you want to express is:
* It has defined semantics for an empty source ranges.
Passing a zero length to `memcpy` is UB per the language standard. This is not an "UB-Sanitizer bug".
💬 ryanofsky commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478021355)
In commit "mining: check witness commitment in submitBlock" (68fc907de93f422fd5566f9066bf16b1239e3a1b)
It seems this comment is not actually true if this function is returning false now, or the comment could at least be clarified.
It also seems like just returning false here on failure without error messages could make it difficult for clients to debug issues. Will more specific errors be logged here? Could they be easily returned?
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478021355)
In commit "mining: check witness commitment in submitBlock" (68fc907de93f422fd5566f9066bf16b1239e3a1b)
It seems this comment is not actually true if this function is returning false now, or the comment could at least be clarified.
It also seems like just returning false here on failure without error messages could make it difficult for clients to debug issues. Will more specific errors be logged here? Could they be easily returned?
💬 ryanofsky commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2477980297)
In commit "mining: check witness commitment in submitBlock" (68fc907de93f422fd5566f9066bf16b1239e3a1b)
It's good to mention the block is modified now but it would seem helpful to provide a little more detail here about what is expected and what is modified here especially since IPC behavior is different from RPC.
You also [mentioned](https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3299129244) that RPC "This is safe for submitted blocks" documentation isn't really true and it wou
...
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2477980297)
In commit "mining: check witness commitment in submitBlock" (68fc907de93f422fd5566f9066bf16b1239e3a1b)
It's good to mention the block is modified now but it would seem helpful to provide a little more detail here about what is expected and what is modified here especially since IPC behavior is different from RPC.
You also [mentioned](https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3299129244) that RPC "This is safe for submitted blocks" documentation isn't really true and it wou
...
💬 fanquake commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3468059025)
https://github.com/bitcoin/bitcoin/actions/runs/18939943748/job/54075967306?pr=33745#step:9:2351:
```bash
test/miner_tests.cpp(70): Entering test suite "miner_tests"
test/miner_tests.cpp(686): Entering test case "CreateNewBlock_validity"
<snip>
2025-10-30T12:08:21.221330Z [test] [validationinterface.cpp:218] [void ValidationSignals::BlockConnected(ChainstateRole, const std::shared_ptr<const CBlock> &, const CBlockIndex *)] [validation] Enqueuing BlockConnected: block hash=000000000019d66
...
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3468059025)
https://github.com/bitcoin/bitcoin/actions/runs/18939943748/job/54075967306?pr=33745#step:9:2351:
```bash
test/miner_tests.cpp(70): Entering test suite "miner_tests"
test/miner_tests.cpp(686): Entering test case "CreateNewBlock_validity"
<snip>
2025-10-30T12:08:21.221330Z [test] [validationinterface.cpp:218] [void ValidationSignals::BlockConnected(ChainstateRole, const std::shared_ptr<const CBlock> &, const CBlockIndex *)] [validation] Enqueuing BlockConnected: block hash=000000000019d66
...
💬 hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3468078851)
> What is the plan for adding CI, as that blocks everything here?
I'm working on it. I have issues with the `sqlite` package that are not reproducible neither in the [nightly builds](https://github.com/hebasto/bitcoin-core-nightly/actions/workflows/windows-gcc-ucrt.yml?query=event%3Aschedule) nor locally.
(https://github.com/bitcoin/bitcoin/pull/33593#issuecomment-3468078851)
> What is the plan for adding CI, as that blocks everything here?
I'm working on it. I have issues with the `sqlite` package that are not reproducible neither in the [nightly builds](https://github.com/hebasto/bitcoin-core-nightly/actions/workflows/windows-gcc-ucrt.yml?query=event%3Aschedule) nor locally.
💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478177222)
I move the change away from here.
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478177222)
I move the change away from here.
💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3468101541)
@TheCharlatan wrote:
> What about the other checked flags? Should we just reset all of them in `AddMerkleRootAndCoinbase`?
Done and pushed, but still checking the CI failure.
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3468101541)
@TheCharlatan wrote:
> What about the other checked flags? Should we just reset all of them in `AddMerkleRootAndCoinbase`?
Done and pushed, but still checking the CI failure.
💬 fanquake commented on pull request "doc: add coverage instrumentation hint to libFuzzer quickstart":
(https://github.com/bitcoin/bitcoin/pull/33536#issuecomment-3468116092)
@frankomosh the PR description needs updating to reflect the current change?
cc @dergoegge @marcofleon
(https://github.com/bitcoin/bitcoin/pull/33536#issuecomment-3468116092)
@frankomosh the PR description needs updating to reflect the current change?
cc @dergoegge @marcofleon
💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3468117727)
I dropped `Assume(pblock->m_checked_witness_commitment);` from `miner.cpp`, since the assumptions seems to be wrong.
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3468117727)
I dropped `Assume(pblock->m_checked_witness_commitment);` from `miner.cpp`, since the assumptions seems to be wrong.
💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478202068)
It behaves the same way as `ProcessNewBlock`, which has the following confusing documentation:
> This only returns after the best known valid
> * block is made active. Note that it does not, however, guarantee that the
> * specific block passed to it has been checked for validity!
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478202068)
It behaves the same way as `ProcessNewBlock`, which has the following confusing documentation:
> This only returns after the best known valid
> * block is made active. Note that it does not, however, guarantee that the
> * specific block passed to it has been checked for validity!
💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478209632)
I do think it would be nice to return clear errors somehow, but that seems a separate issue.
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478209632)
I do think it would be nice to return clear errors somehow, but that seems a separate issue.
💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478210721)
Done
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478210721)
Done
💬 fanquake commented on issue "RFC: Add multiprocess fuzz target":
(https://github.com/bitcoin/bitcoin/issues/23015#issuecomment-3468145119)
Note that capnp has it's own detection for if it's being fuzzed, which was added as part of its oss-fuzz integration (https://github.com/capnproto/capnproto/pull/1188). This will likely need to be patched out/worked around at some point. See https://github.com/google/oss-fuzz/pull/14203.
(https://github.com/bitcoin/bitcoin/issues/23015#issuecomment-3468145119)
Note that capnp has it's own detection for if it's being fuzzed, which was added as part of its oss-fuzz integration (https://github.com/capnproto/capnproto/pull/1188). This will likely need to be patched out/worked around at some point. See https://github.com/google/oss-fuzz/pull/14203.
👍 TheCharlatan approved a pull request: "test: Add bitcoin-chainstate test for assumeutxo functionality"
(https://github.com/bitcoin/bitcoin/pull/33728#pullrequestreview-3399808856)
ACK 3e7d272b2747f1937010b8fc1fefafcfa0ce3213
(https://github.com/bitcoin/bitcoin/pull/33728#pullrequestreview-3399808856)
ACK 3e7d272b2747f1937010b8fc1fefafcfa0ce3213
💬 maflcko commented on pull request "fuzz: refactor memcpy to std::ranges::copy to work around ubsan warn":
(https://github.com/bitcoin/bitcoin/pull/33743#issuecomment-3468260631)
Yeah, I guess it is too early to call this a bug, but the language standard has been changed, see https://github.com/bitcoin/bitcoin/pull/33644#issuecomment-3430890579.
I've used your wording for now.
(https://github.com/bitcoin/bitcoin/pull/33743#issuecomment-3468260631)
Yeah, I guess it is too early to call this a bug, but the language standard has been changed, see https://github.com/bitcoin/bitcoin/pull/33644#issuecomment-3430890579.
I've used your wording for now.
💬 maflcko commented on issue "valgrind: Conditional jump or move depends on uninitialised value(s)":
(https://github.com/bitcoin/bitcoin/issues/29635#issuecomment-3468267477)
I think this is hopeless, even with `-O1`, using the following diff, it fails.
```diff
sh-5.2# git log -1 --oneline
72511fd (grafted, HEAD -> master, origin/master, origin/HEAD) Merge bitcoin/bitcoin#33555: build: Bump clang minimum supported version to 17
sh-5.2# git diff
diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh
index d6c4575..b64648f 100755
--- a/ci/test/00_setup_env_native_valgrind.sh
+++ b/ci/test/00_setup_env_native_valgrind.sh
@@ -8,7
...
(https://github.com/bitcoin/bitcoin/issues/29635#issuecomment-3468267477)
I think this is hopeless, even with `-O1`, using the following diff, it fails.
```diff
sh-5.2# git log -1 --oneline
72511fd (grafted, HEAD -> master, origin/master, origin/HEAD) Merge bitcoin/bitcoin#33555: build: Bump clang minimum supported version to 17
sh-5.2# git diff
diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh
index d6c4575..b64648f 100755
--- a/ci/test/00_setup_env_native_valgrind.sh
+++ b/ci/test/00_setup_env_native_valgrind.sh
@@ -8,7
...