💬 ryanofsky commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1282083953)
re: https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1282008318

> Guess that you wrote this because you saw it in a unit test?

No, it I believe it happens pretty reliably when you use `-reindex-chainstate`. I saw it reliably when I ran the `test/functional/feature_coinstatsindex.py` "[Test that the index works with -reindex-chainstate](https://github.com/bitcoin/bitcoin/blob/2fa60f0b683cefd7956273986dafe3bde00c98fd/test/functional/feature_coinstatsindex.py#L238-L240)" test and had
...

💬 ryanofsky commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1282110542)
re: https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1282048353

> Isn't this conflicting with the `StartBackgroundSync()` call?
>
> Let's say that there are new blocks on the notifications queue at this point. By not setting the `m_synced` flag, we allow `ThreadSync()` to start.

Yes, that's a good point. I forgot that the m_synced flag is not just used to process notifications, but also to determine whether to start the sync thread.

It's hard to imagine this being a problem
...

💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1282141206)
Fixed

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1662535127)
> Since dumping should be a read-only operation, I think it's better to use the read-only database since it definitely won't write to it like BDB does.

But it could produce a corrupt dump, which someone might rely on for their backup (even if they shouldn't).

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1282150028)
Nice! I'll run the fuzzer against that...

💬 jonatack commented on pull request "rpc, util: avoid string copies in ParseHash/ParseHex functions":
(https://github.com/bitcoin/bitcoin/pull/28172#discussion_r1282150661)
Pushed an update to use `string_view` instead.

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