Bitcoin Core Github
44 subscribers
121K links
Download Telegram
📝 l0rinc opened a pull request: "refactor,test: follow-ups to multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/33039)
Follow up for https://github.com/bitcoin/bitcoin/pull/31144
Applied the remaining comments in separate commits - except for the last one where I could group them.
Please see the commit messages for more context.
🤔 l0rinc reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3043917032)
Added a follow-up with the remaining nits, see: https://github.com/bitcoin/bitcoin/pull/33039
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223278879)
Done, together with the bit_cast -> reinterpret_cast change and comment renames, see [`fee3048` (#33039)](https://github.com/bitcoin/bitcoin/pull/33039/commits/fee3048fcc4972ba02127a2a4ef75524c5be275d)
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223178526)
I'm usually a fan of small and focused tests that complement each other - instead of end-to-end ones that test everything. But I don't mind merging these either, done something similar in a separate follow-up PR: [`a17d820` (#33039)](https://github.com/bitcoin/bitcoin/pull/33039/commits/a17d8202c36abf8a17fb8736e05f318422a3c7fb)
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223255423)
Added it to the test suite: [`2dea045` (#33039)](https://github.com/bitcoin/bitcoin/pull/33039/commits/2dea0454254180d79464dc6afd3312b1caf369a7)
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223239857)
Sure, done in a separate PR: [`e5b1b7c` (#33039)](https://github.com/bitcoin/bitcoin/pull/33039/commits/e5b1b7c5577ee36b5bcfb6c02b92da88455411e9)
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223234425)
Done in a separate PR, thanks: [`298bf95` (#33039)](https://github.com/bitcoin/bitcoin/pull/33039/commits/298bf9510578263a1439513729e5ff955a453437)
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223272055)
I don't mind writing an `Obfuscation` object directly, it does make more sense to make that symmetric with the read.
And if you still don't like reading back what we wrote (which would exercise the same route that we'd take otherwise, seems better to me), we can also just assign it directly.
Done in [`62aa7b9` (#33039)](https://github.com/bitcoin/bitcoin/pull/33039/commits/62aa7b9fabf03872fe0905076b6d0b275747d188)
👍 l0rinc approved a pull request: "validation: ensure assumevalid is always used during reindex"
(https://github.com/bitcoin/bitcoin/pull/31615#pullrequestreview-3044142789)
ACK 89b5b607e3380ff2cf03d8199c70e655e8c265cb

Consider unifying the way your refer to the first block that fulfills the assumevalid condition
💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2223351892)
Commit message is inconsistent with comments: `assumedvalid block` is written in your code as `assumed valid block`. I would be fine with `assumevalid block` as well, as long as they're consistent.

nit: sentence ends in `full block yet` - we could add a fullstop.
💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2223343368)
To be consistent with previous line:
```suggestion
// if the previous IBD run was interrupted before it downloaded the assumed valid block.
```
💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2223342500)
```suggestion
// If no blocks were imported, ActivateBestChain will have nothing to do
```
💬 maflcko commented on pull request "fuzz: add mempool_dag fuzzer for transaction dependency testing":
(https://github.com/bitcoin/bitcoin/pull/33038#discussion_r2223364583)
this is just copy-pasted from `src/test/fuzz/tx_pool.cpp`, same for the function above, with slight modifications.

it would be good to explain what exactly you are trying to test and why the existing fuzz targets are insufficient. Then, it would be good to provide coverage reports to show you have achieved your goal. Finally, it would be good to put this in the existing `src/test/fuzz/tx_pool.cpp` file, so that shared code can be re-used.
💬 achow101 commented on pull request "test: check tx is final when there is no locktime":
(https://github.com/bitcoin/bitcoin/pull/33030#issuecomment-3104091020)
ACK 065e42976a70738770af256da810ddc1316a4496
💬 l0rinc commented on pull request "p2p: rename GetAddresses -> GetAddressesUnsafe":
(https://github.com/bitcoin/bitcoin/pull/32994#issuecomment-3104099469)
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2223414637)
Thanks, will do this on the next push
🚀 achow101 merged a pull request: "test: check tx is final when there is no locktime"
(https://github.com/bitcoin/bitcoin/pull/33030)
💬 achow101 commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3104201511)
ACK 06ab3a394ade1e0d4fdb1fef636ff0d6bad71948

The test isn't that slow for me, but this is still a significant improvement.