💬 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?

💬 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
...

💬 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
...

💬 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.

💬 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.

💬 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.

💬 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

💬 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.

💬 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!

💬 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.

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(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.

👍 TheCharlatan approved a pull request: "test: Add bitcoin-chainstate test for assumeutxo functionality"
(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.

💬 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
...

✅ maflcko closed a pull request: "doc: Document valgrind aarch64 clang workaround"
(https://github.com/bitcoin/bitcoin/pull/33742)

💬 maflcko commented on pull request "doc: Document valgrind aarch64 clang workaround":
(https://github.com/bitcoin/bitcoin/pull/33742#issuecomment-3468281402)
Closing for now

💬 GregTonoski commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-3468291452)
> For the record, this pull-req wasn't my idea. I was asked to open it by an active Core dev because entities like Citrea are using unprunable outputs instead of OP_Return, due to the size limits. - @petertodd, https://stacker.news/items/971277?commentId=971434

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478319678)
Thinking about adding the following to this comment:

```cpp
/** Update uncommitted block structures (currently: only the witness reserved
* value). This is safe for submitted blocks as long as they honor
* default_witness_commitment from the template. */
void UpdateUncommittedBlockStructures(CBlock& block, const CBlockIndex* pindexPrev) const;
```

💬 instagibbs commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3468315631)
@Crypt-iQ `BlockEncoding.*` benchmarks should cover this up to 50k. Quickly tested 100k, and it seems ~linear slowdown.