📝 josibake opened a pull request: "Silent Payments: implement sending"
(https://github.com/bitcoin/bitcoin/pull/28201)
This PR depends on #28122 and is marked as a draft until it is merged. Commits up to https://github.com/bitcoin/bitcoin/commit/8ee791e7b34a354ce6835b968fe12924b8908d7c belong to #28122; please review those commits on #28122

## Sending

_Description coming soon_

💬 1ma commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1662568232)
So there's this thing called AWS S3... Bitcoin is about moving sats around, not storing arbitrary bytes on the blockchain.

Enacting this change of policy would be reason enough for me to stop updating Bitcoin Core forever. If anything node runners are asking for ways to filter the recent tidal wave of spam while it is still unconfirmed, not surrender to it.

Concept NACK.

📝 josibake opened a pull request: "Silent Payments: implement receiving"
(https://github.com/bitcoin/bitcoin/pull/28202)
This PR depends on #28122 and is marked as a draft until it is merged. Commits up to https://github.com/bitcoin/bitcoin/commit/8ee791e7b34a354ce6835b968fe12924b8908d7c belong to #28122; please review those commits on #28122

## Receiving

_Description coming soon_

💬 murchandamus commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1282093736)
Can someone remind me wtf regtest has a different hrp than testnet and signet?

If each of the four had a different one, I’d get it, but why does regtest have a different one than the other two testing networks?

Unless I’m forgetting about some good reason for that to be there, I might recommend that you just use `tsp` for all three

📝 josibake converted_to_draft a pull request: "Silent Payments: send and receive"
(https://github.com/bitcoin/bitcoin/pull/27827)
UPDATE: In an attempt to make reviewing a bit more sane, I'm breaking this up into a few smaller PRs, but will keep this one open as the parent PR and keep it rebased on the child PRs.

Marking as a draft for now. The main purpose of having this PR is to track progress on child PRs and also have an easy way to compile `bitcoind` with both send and receive support for testing. Additionally, I'll be adding more functional tests to this PR since its much easier to test when `bitcoind` can both se
...

👍 ryanofsky approved a pull request: "refactor: Remove unused includes from wallet.cpp"
(https://github.com/bitcoin/bitcoin/pull/28200#pullrequestreview-1559456848)
Code review ACK ccccbd8dd0f5cd74a744e74608c4ea102ec33fd1

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1282196740)
So far my corpus doesn't make it here, as tested by sprinkling `assert(false)`. I may indeed need some help with a real wallet.

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1282211461)
I simply copied a `wallet.dat` into the corpus directory. That resulted in a bunch of extra corpus entities and almost immediately a crash.

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1282214518)
Though the file seems to get ignored when trying to merge.

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1282220244)
Done

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1282227280)
Ah, figured it out! Will push some new entries to the qa-assets PR that should reproduce the crash.

💬 jonatack commented on pull request "rpc, util: use string_view for passing string literals to Parse{Hash,Hex}":
(https://github.com/bitcoin/bitcoin/pull/28172#issuecomment-1662703920)
Updated to pass string literals to the `Parse{Hash,Hex}` functions by value as `string_view` rather than `std::string`. Thank you for the helpful feedback!

<details><summary>code demonstrating the perf improvement</summary><p>

```cpp
// passing-string-literals.cpp
//
// $ g++ -std=c++17 passing-string-literals.cpp && ./a.out
// StringByValue("string literal") took 1426 cycles
// StringByConstRef("string literal") took 1412 cycles
// StringViewByValue("string literal") took 612 cycles
...

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1662720996)
I tried adding `if (bdb_ro_err) return;` above `#ifdef USE_BDB` (although that does reduce coverage). I also added in the second catch:

```cpp
// We are not interested in fuzzing the original BDB implementation
if (std::string(e.what()) == "BerkeleyDatabase: Error 22, can't open database fuzzed_wallet.dat") return;
```

That helps a bit, but still leads to a "SEGV on unknown address 0x000000000000" in `DumpWallet` very quickly. I haven't looked at what the fuzzer came up with. Perhaps it
...

📝 martinus opened a pull request: "refactor: serialization simplifications"
(https://github.com/bitcoin/bitcoin/pull/28203)
This simplifies the serialization code a bit and should also make it a bit faster.

* use fold expressions instead of recursive calls. This simplifies the code, makes it most likely faster because it reduces the number of function calls, and compiles faster because there are fewer template instantiations.

* use `if constexpr` instead of unnecessarily creating a temporary object only to call the right overload. This is used for `std::vector` and `prevector` serialization.

🤔 instagibbs reviewed a pull request: "test: tx orphan handling"
(https://github.com/bitcoin/bitcoin/pull/28199#pullrequestreview-1559366940)
moar coverage good

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282251680)
took me a second, just assert it's not in mempool for those quickly reading

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282234657)
```suggestion
# The parent should be requested since the unstripped wtxid would differ. Delayed because it's by txid and this is not a preferred relay peer.
```

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282208201)
Stick the constant inside `cleanup` and move the comment for `cleanup`, since it explains what the whole thing is doing.

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282243358)
can't we just reconsider it again?

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282131242)
comment block seems extraneous if there's logging for each test