💬 furszy 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_r1282048353)
In ba30b60b:

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.
This former one might have some work to do if some of the queued blocks notifications were processed in-between `Init()` and `StartBackgroundSync()`. Then, while this is being done, the validation queue thread might end up setting `m_synced=true`. Which would enable the
...

💬 MarcoFalke commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1662427090)
Thanks, force pushed to fix the linter and made the commit hash start with cccc

💬 brandonblack commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#discussion_r1282089319)
Might be better to add a comment above line 29 a la
```
# both nodes disable full-rbf to test BIP125 signaling
```

and then change the comment on line 38 to
```
# second node has default mempool limits
```

🤔 brandonblack reviewed a pull request: "policy: Enable full-rbf by default"
(https://github.com/bitcoin/bitcoin/pull/28132#pullrequestreview-1559299935)
utACK 024403e95d76f824c58851d84468c693bf0cbeae

💬 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