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

💬 marcofleon commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2478495348)
Non-blocking I'd say, so could be a followup once it's figured out.

💬 ajtowns commented on pull request "contrib: refactor: dedup deserialization routines in utxo-to-sqlite script":
(https://github.com/bitcoin/bitcoin/pull/32116#issuecomment-3468660227)
> Right. A cleaner approach to deduplication would be introduce a python utility module that's used by both the test framework and the tools (but is in a third location), this makes it clear that there's an interface at least.

Concept ACK! https://github.com/jamesob/verystable is something like this, arguably. It would also be nice to export it onto pypi (or whatever) for easy quick-and-dirty python/bitcoin scripting.

This PR lgtm, tested ACK f535b2fe63255175f7a35882599f9b6b83ac25f1

💬 fjahr commented on pull request "test: add valid tx test with minimum-sized ECDSA signature (8 bytes DER-encoded)":
(https://github.com/bitcoin/bitcoin/pull/32924#issuecomment-3468674950)
tACK 5fa81e239a39d161a6d5aba7bcc7e1f22a5be777

Reviewed code and verified the behavior with the suggestions in the PR description.

💬 ajtowns commented on pull request "contrib: refactor: dedup deserialization routines in utxo-to-sqlite script":
(https://github.com/bitcoin/bitcoin/pull/32116#issuecomment-3468685164)
> Right. A cleaner approach to deduplication would be introduce a python utility module that's used by both the test framework and the tools (but is in a third location), this makes it clear that there's an interface at least.

Big agree on this, eager to review if someone wants to do it. https://github.com/jamesob/verystable or https://pypi.org/project/verystable/ is somewhat like this, except external. It would also be nice to export it onto pypi (or whatever) for easy quick-and-dirty python
...

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2478627740)
I think outbound connections are getting disconnected for some reason, latest coverage is [here](https://crypt-iq.github.io/fuzz_coverage_reports/cmpctblock-aflpp-inputs-10292025/).