👋 sipa's pull request is ready for review: "blockstorage: XOR blocksdir *.dat files"
(https://github.com/bitcoin/bitcoin/pull/28052)

💬 sipa commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1627435086)
Oops, I clocked wrong.

I just meant to Concept ACK.

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