💬 hebasto commented on pull request "guix: import/sync python-lief (0.12.3) package definition from upstream":
(https://github.com/bitcoin/bitcoin/pull/27296#issuecomment-1479266165)
Concept ACK.

💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1144539820)
> Maybe for a follow-up to avoid another round of re-review?

Yes, please :)

💬 darosior commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1479296136)
Concept ACK. Have you looked into a fuzz target for this parser?

💬 fanquake commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1144580869)
In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: Make these Doxygen comments?

💬 fanquake commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1144582632)
In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: These could be
```suggestion
constexpr static size_t RFC8439_KEYLEN{32};
```

💬 fanquake commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1144584874)
In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: nit use c++ headers, i.e `<cassert>`

💬 fanquake commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1144588967)
Please don't add `for xyz` comments. They are unmaintainable, and if someone wants to know why an include is used, they can look at IWYU etc. Note that you could also add newly-added files to `ci/test/06-script_b.sh` to have the includes checked.

💬 fanquake commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1144594272)
```suggestion
std::memcpy(header_and_contents.data(), &flags, BIP324_HEADER_LEN);
```
Same for memset etc.

💬 fanquake commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1144580119)
In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620:
It is fairly awkward to introduce a second copy of `timingsafe_bcmp` (one already in `chacha_poly_aead`), with a different name, and broken #ifdef logic, just to then rename it when you delete the original `timingsafe_bcmp` in a later commit. Although I guess re-using/extracting `timingsafe_bcmp` early from the to-be-deleted file is also a bit awkward.

💬 fanquake commented on pull request "BIP324: Cipher suite":
(https://github.com/bitcoin/bitcoin/pull/25361#discussion_r1144586412)
nit: additional newline

💬 fanquake commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1144620929)
Ok. We'll push this to a followup.

💬 darosior commented on issue "Allow for rescan using block filters with pruned node":
(https://github.com/bitcoin/bitcoin/issues/21267#issuecomment-1479349534)
@furszy are you still working on this?

💬 darosior commented on issue "Manual-pruning cursor rewind":
(https://github.com/bitcoin/bitcoin/issues/19807#issuecomment-1479351248)
Slightly related: https://github.com/bitcoin/bitcoin/issues/21267.

🚀 fanquake merged a pull request: "refactor: Use move semantics instead of custom swap functions"
(https://github.com/bitcoin/bitcoin/pull/26749)

💬 fanquake commented on pull request "test: add support for all networks in `deserialize_v2`":
(https://github.com/bitcoin/bitcoin/pull/27295#issuecomment-1479382280)
https://cirrus-ci.com/task/6600092300345344
```bash
test 2023-03-21T23:55:59.273000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
self.run_test()

...

💬 MarcoFalke commented on pull request "test: add support for all networks in `deserialize_v2`":
(https://github.com/bitcoin/bitcoin/pull/27295#issuecomment-1479385710)
@fanquake See https://github.com/bitcoin/bitcoin/issues/18623 ?

💬 ismaelsadeeq commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1144656973)
Thanks for the thorough review Josi.
I will fix that.
This means I have to check in `bech32_to_bytes `also for a None version value after so as not to call bytearray(payload) with None.

💬 ajtowns commented on pull request "refactor: Move chain names to the kernel namespace":
(https://github.com/bitcoin/bitcoin/pull/27294#discussion_r1144661496)
Put the declaration in the header (eg `extern const std::string MAIN;`) and the definition in the corresponding cpp file (`const std::string MAIN{"main"};`)

📝 MarcoFalke opened a pull request: "test: Remove unused Check* default constructors"
(https://github.com/bitcoin/bitcoin/pull/27297)
They are no longer needed after the removal of `swap`, see https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1144532693

Also, flatten a redundant `if` check.

💬 ajtowns commented on pull request "refactor: Move chain names to the kernel namespace":
(https://github.com/bitcoin/bitcoin/pull/27294#discussion_r1144667808)
Adding/moving headers is usually pretty benign -- if you have an extra header it just makes compilation slightly slower and can be caught by iwyu, if you miss a header, it just won't compile. But changing code is where security bugs can sneak in, and if you're touching 100 lines all over the place, having an automated check that nothing's going wrong seems like a big win.