✅ maflcko closed a pull request: "doc: Document valgrind aarch64 clang workaround"
(https://github.com/bitcoin/bitcoin/pull/33742)
(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
(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
(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;
```
(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.
(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
(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;
+
...
(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.
(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.
(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.
(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
(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.
(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
...
(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/).
(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/).
👋 fanquake's pull request is ready for review: "doc: update Guix INSTALL.md"
(https://github.com/bitcoin/bitcoin/pull/33574)
(https://github.com/bitcoin/bitcoin/pull/33574)
💬 ajtowns commented on pull request "contrib: utxo_to_sqlite.py: add option to store txid/spk as BLOBs":
(https://github.com/bitcoin/bitcoin/pull/32621#issuecomment-3468708041)
Conflicts with #32116 ; let's get that merged first?
(https://github.com/bitcoin/bitcoin/pull/32621#issuecomment-3468708041)
Conflicts with #32116 ; let's get that merged first?
💬 fanquake commented on pull request "doc: update Guix INSTALL.md":
(https://github.com/bitcoin/bitcoin/pull/33574#issuecomment-3468712881)
Updated the PR description, and added 1.5.0 release. Marked this as reviewable, as I think its fine drop the expectations section from 2021.
(https://github.com/bitcoin/bitcoin/pull/33574#issuecomment-3468712881)
Updated the PR description, and added 1.5.0 release. Marked this as reviewable, as I think its fine drop the expectations section from 2021.
👍 stickies-v approved a pull request: "test: Use same rpc timeout for authproxy and cli"
(https://github.com/bitcoin/bitcoin/pull/33698#pullrequestreview-3400367122)
tACK 66667d6512294fd5dd02161b7c68c19af0865865
Rationale makes sense, tested old and new behaviour, code LGTM.
(https://github.com/bitcoin/bitcoin/pull/33698#pullrequestreview-3400367122)
tACK 66667d6512294fd5dd02161b7c68c19af0865865
Rationale makes sense, tested old and new behaviour, code LGTM.
💬 fanquake commented on pull request "ci: Add missing python3-dev package for riscv64":
(https://github.com/bitcoin/bitcoin/pull/33746#issuecomment-3468717752)
ACK fae9235d39a9976a725cfc6f32cfacb1bdfcf7a3 - did not test.
(https://github.com/bitcoin/bitcoin/pull/33746#issuecomment-3468717752)
ACK fae9235d39a9976a725cfc6f32cfacb1bdfcf7a3 - did not test.
🤔 l0rinc requested changes to a pull request: "http: replace WorkQueue and single threads handling for ThreadPool"
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3377790750)
I have started reviewing this PR but have only finished the first commit (e1eb4cd3a5eb192cd6d9ee5d255688c06ab2089a).
The depth of tests gives me hope that this transition will be smooth, the tests cover most of the functionality, even a few corner cases I haven't thought of!
It seemed, however, that I had more to say about that than anticipated. I didn't even get to reviewing the threadpool properly, just most of the tests.
I know receiving this amount of feedback can be daunting, but we both
...
(https://github.com/bitcoin/bitcoin/pull/33689#pullrequestreview-3377790750)
I have started reviewing this PR but have only finished the first commit (e1eb4cd3a5eb192cd6d9ee5d255688c06ab2089a).
The depth of tests gives me hope that this transition will be smooth, the tests cover most of the functionality, even a few corner cases I haven't thought of!
It seemed, however, that I had more to say about that than anticipated. I didn't even get to reviewing the threadpool properly, just most of the tests.
I know receiving this amount of feedback can be daunting, but we both
...