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

💬 yuvicc commented on pull request "test: Add bitcoin-chainstate test for assumeutxo functionality":
(https://github.com/bitcoin/bitcoin/pull/33728#issuecomment-3468350669)
Concept ACK

💬 ajtowns commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#issuecomment-3468367525)
> the second peer can endlessly send CMPCTBLOCK's

If we're concerned about that, seems better to fix it directly?

```diff
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -4412,6 +4412,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
while (range_flight.first != range_flight.second) {
if (range_flight.first->second.first == pfrom.GetId()) {
requested_block_from_this_peer = true;
+
...

💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#discussion_r2478444690)
Done. I also clarified which fields are modified.

🤔 marcofleon reviewed a pull request: "fuzz: refactor memcpy to std::ranges::copy to work around ubsan warn"
(https://github.com/bitcoin/bitcoin/pull/33743#pullrequestreview-3400116871)
tACK fa4b52bd16189d40761c5976b8427e30779aba23

Tested this branch and didn't see the null pointer error. The change to `std::byte` is for modernization and the change to `std::ranges` addresses the issue.