🤔 TheCharlatan reviewed a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1520702348)
Concept ACK

💬 TheCharlatan commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1257291194)
I was looking at the initialization order here and noticed that I might have introduced more unsafe behavior here in e2d680a32d757de0ef8eb836047a0daa1d82e3c4. These handlers are registered in `AppInitBasicSetup`. But if you look in bicoind.cpp, the kernel context is created after `AppInitBasicSetup` is called.

💬 TheCharlatan commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1257259643)
Why not use `EnsureAnyNodeContext` here as done in many other places?

💬 TheCharlatan commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1257286292)
I think we might have introduced a bug here before. This may be possible to trigger when running the GUI before the kernel context is initialized with `app.baseInitialize()`. We could just move it to after `baseInitialize` (see my [patch here](https://github.com/TheCharlatan/bitcoin/commit/2df7a669a07be01ef2cce0d22891617bc19060d7)), but I am not sure if that would be correct either. Maybe `app.baseInitialize()` can be moved up a bit instead?

💬 sipa commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1257381343)
`clang-tidy --checks=bugprone-argument-comment` catches if the names in such comments mismatch the formal parameter names of the called function, but (I just tested) it works both with and without the spaces.

💬 sipa commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1257388744)
> since an unconfirmed parent's sequence number should before that of its child.

What if a reorg happens, which adds a formerly-confirmed parent of an unconfirmed transaction back to the mempool?

I don't think this example is particularly relevant in this situation (in such circumstances suboptimal relay is to be expected anyway), but perhaps it's enough to make a blanket assumption that parent.entry_sequence < child.entry_sequence.

📝 luke-jr opened a pull request: "Bugfix: net_processing: Restore "Already requested" error for FetchBlock"
(https://github.com/bitcoin/bitcoin/pull/28055)
Regression introduced by #27626

Adds new tests

📝 MarcoFalke converted_to_draft a pull request: "blockstorage: XOR blocksdir *.dat files"
(https://github.com/bitcoin/bitcoin/pull/28052)
Currently the *.dat files in the blocksdir store the data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan them and move them into quarantine, or delete them, or corrupt them. This may cause Bitcoin Core to fail a reorg, or fail to reply to P2P or RPC block requests.

Fix this, similar to https://github.com/bitcoin/bitcoin/pull/6650, by rolling a ran
...

📝 ItIsOHM opened a pull request: "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998"
(https://github.com/bitcoin/bitcoin/pull/28056)
This PR will add the optional parameters `longpollid` and `data` to `template_request` as they were missing when calling `help getblocktemplate` in RPCHelpMan.

I request the maintainers to review this and let me know about any mistakes in the descriptions of the parameters.

This PR refers to the issue #27998

💬 ItIsOHM commented on issue "rpc: doc: RPCHelpMan missing request parameters for getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/27998#issuecomment-1627627747)
Hello @stickies-v, sorry for the delay. I recently made a PR (https://github.com/bitcoin/bitcoin/pull/28056) and changed the description of the parameters, taking help from BIP 22 and BIP 23 as you suggested. But I am still unsure if it's a totally correct description or not, hence I have marked it as WIP.

I apologize if these changes aren't correct as I'm just a beginner here.

💬 MarcoFalke commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1627664179)
This doesn't compile. Ideally, you compile and test any changes locally before opening a pull request.

⚠️ MarcoFalke opened an issue: "migratewallet crashes (wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wallet::LegacyScriptPubKeyMan::MigrateToDescriptor(): Assertion `IsMine(desc_spk) != ISMINE_NO' failed.)"
(https://github.com/bitcoin/bitcoin/issues/28057)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

crash

### Expected behaviour

no crash

### Steps to reproduce

* Compile 79e8247ddb166f9b980f40249b7372a502402a4d
* Type `migratewallet`

### Relevant log output

```
wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wallet::LegacyScriptPubKeyMan::MigrateToDescriptor(): Assertion `IsMine(desc_spk) != ISMINE_NO' failed.
Aborted (core dumped)


### How did you obtain Bitcoi
...

🤔 theStack reviewed a pull request: "Add support for RFC8439 variant of ChaCha20"
(https://github.com/bitcoin/bitcoin/pull/27985#pullrequestreview-1520883441)
Nice and indeed much simpler now, the actual diff is smaller than I expected. Will re-review tomorrow.

Meanwhile, I thought it would be nice to have a test case triggering the 32-bit block counter overflow to check for >256 GiB output compatibility and wrote one (test data is created with PyCryptodome, see commit message), maybe it's worthwhile to include it here: https://github.com/theStack/bitcoin/commit/61b41ad16e35306485a1db508d624d3ddab0ca0a (the next commit https://github.com/theStack/b
...

💬 MarcoFalke commented on issue "migratewallet crashes (wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wallet::LegacyScriptPubKeyMan::MigrateToDescriptor(): Assertion `IsMine(desc_spk) != ISMINE_NO' failed.)":
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1627675932)
Now trying to start the program just fails on startup:

```
Error: Unrecognized descriptor found. Loading wallet ....../wallet.dat

The wallet might had been created on a newer version.
Please try running the latest software version.

👍 TheCharlatan approved a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1520884392)
Nice, ACK 4223d80bdae0d265e67280ea8853b184ff0ced13

Nit can be ignored.

💬 TheCharlatan commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1257458241)
Nit: `const CBlockIndex* block_to_test = start_block ? start_block : active_chain.Genesis();`

💬 theStack commented on pull request "test: Check expected_stderr after stop":
(https://github.com/bitcoin/bitcoin/pull/28028#discussion_r1257463070)
nit, logically equivalent, but likely less ambiguous for the reader's eyes:
```suggestion
assert (not expected_stderr) or wait_until_stopped # Must wait to check stderr
```

🤔 theStack reviewed a pull request: "test: Check expected_stderr after stop"
(https://github.com/bitcoin/bitcoin/pull/28028#pullrequestreview-1520888792)
Code looks good to me, will test locally tomorrow.

💬 sipa commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1627726804)
@theStack Cherrypicked the first commit. I don't think the second one adds that much, as presumably there is no way the output can be correct if the internal state is wrong.

💬 furszy commented on issue "migratewallet crashes (wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wallet::LegacyScriptPubKeyMan::MigrateToDescriptor(): Assertion `IsMine(desc_spk) != ISMINE_NO' failed.)":
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1627746019)
To recover from the error, use the wallet backup. Can find it inside the directory of the wallet that you are trying to migrate e.g. `/wallets/wallet_name/wallet_name-<timestamp>.legacy.bak`.

The issue seems to be related to one of your imported scripts. Would be good to gather more information about the script type. So we can check under which circumstances the legacy spkm `IsMine` return false for a script contained by the wallet.

If you can, try applying this patch and re-run the migrat
...