💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1144532693)
Same commit: Can remove the now unused constructors for all other structs as well? See diff, plus unrelated cleanup: (Maybe for a follow-up to avoid another round of re-review?)


```diff
diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp
index 012b0eb321..975dace633 100644
--- a/src/test/checkqueue_tests.cpp
+++ b/src/test/checkqueue_tests.cpp
@@ -57,7 +57,6 @@ struct FakeCheckCheckCompletion {
struct FailingCheck {
bool fails;
FailingCheck(bool _fa
...

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