💬 MarcoFalke commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1257088164)
The reason why I left the comment is that `randbytes` is a template function and your wrapper function isn't one, so I fail to see the benefits of the wrapper function. It would be good to mention at least one benefit. Otherwise it seems odd to change code in one direction and then create a follow-up to revert the exact code changes and do some other change.

🤔 MarcoFalke reviewed a pull request: "kernel: Rm ShutdownRequested and AbortNode from validation code."
(https://github.com/bitcoin/bitcoin/pull/27861#pullrequestreview-1518337119)
Concept ACK, left some questions.



Concept ACK 6eb33bd0c21b3e075fbab596351cacafdc947472 🗼

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
...

💬 MarcoFalke commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1257118235)
in 6eb33bd0c21b3e075fbab596351cacafdc947472: What is this?

It would be good to split this up in it's own commit, so that it is possible to add a description as to why the changes are needed and beneficial.

Currently it looks like a mistake, because my understanding is that it will just lead to a linker error to call this function somewhere outside of base.cpp

💬 MarcoFalke commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1255459930)
Would it make sense to document that all of these may throw, except for `bool()`?

💬 MarcoFalke commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1255496707)
Also, it would be good to explain why in the following commits it is fine to then ignore the exceptions and no longer catch them.

💬 MarcoFalke commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1257121025)
nit in 6eb33bd0c21b3e075fbab596351cacafdc947472: Why are you replacing cassert with assert.h?

💬 MarcoFalke commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1257126777)
Ah nvm. I see it is private and probably needed to access the `m_...` member.

💬 nopara73 commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1626903191)
It's good enough.

📝 MarcoFalke opened a pull request: "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
...

💬 MarcoFalke commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1257196740)
Looks like this is the only option that is required by the fuzz target, so maybe remove the others for now to hopefully reduce the number of conflicting pulls and remove the dependency on the other pull?

💬 MarcoFalke commented on pull request "test: indexes, fix on error infinite loop":
(https://github.com/bitcoin/bitcoin/pull/28044#issuecomment-1627051897)
Can be tested on current master with:

```diff
diff --git a/src/index/base.cpp b/src/index/base.cpp
index cf07cae286..2987eeb97b 100644
--- a/src/index/base.cpp
+++ b/src/index/base.cpp
@@ -234,6 +234,7 @@ void BaseIndex::ThreadSync()
__func__, pindex->GetBlockHash().ToString());
return;
}
+ return FatalErrorf("foobar");
}
}

```

Result on master:

```
$ ./src/test/test_bitcoin -t blockfilter
...

📝 TheCharlatan opened a pull request: "refactor: Move stopafterblockimport option out of blockstorage"
(https://github.com/bitcoin/bitcoin/pull/28053)
This has the benefit of moving this StartShutdown call out of the blockstorage file and thus out of the kernel's responsibility. The user can now decide if he wants to start shutdown / interrupt after a block import or not.

Once https://github.com/bitcoin/bitcoin/pull/27607 is merged (splitting up the block import and `LoadMempool` steps), the `return_after_import` bool can be removed.

This also simplifies https://github.com/bitcoin/bitcoin/pull/28048, making it one fewer shutdown call to
...

💬 MarcoFalke commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#issuecomment-1627094953)
I have the same question here: https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1255496707

💬 TheCharlatan commented on pull request "kernel: Remove shutdown globals from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1627096093)
Closing this PR in favor of:
* #28053 Handling shutdown with `-stopafterblockimport`
* https://github.com/bitcoin/bitcoin/pull/28048 Handling shutdown with `-stopatheight`
* https://github.com/bitcoin/bitcoin/pull/27866 Handling blockstorage flush errors
* https://github.com/bitcoin/bitcoin/pull/28051 Getting rid of shutdown code

✅ TheCharlatan closed a pull request: "kernel: Remove shutdown globals from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27711)

💬 TheCharlatan commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#issuecomment-1627142023)
Re https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1520042712
> Maybe the simplest path to not conflict much could be to provide ThreadImport with a "return_after_import" bool arg (true when -stopafterblockimport is set)

Thanks @furszy, opened https://github.com/bitcoin/bitcoin/pull/28053.

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1627212544)
@achow101 I added `IsNotification()` and a test. Also rebased the PR with more atomic commits for hopefully easier reviewing. As far as 500, that should only be returned by a "legacy" json rpc request. The legacy protocol should be unchanged by this PR. Unfortunately if the request is unparseable we can not read the `jsonrpc:"2.0"` identifier so the default is to assume legacy.

⚠️ TNTBOMBOM opened an issue: "Bitcoin core hidden services link down/doesnt work"
(https://github.com/bitcoin/bitcoin/issues/28054)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Bitcoin core hidden services website entry not functional.

### Expected behaviour

It should work/connect.

### Steps to reproduce

Just visit the URL with your TB. or you can test the [onion URL](http://6hasakffvppilxgehrswmffqurlcjjjhd76jgvaqmsg6ul25s7t3rzyd.onion/) here:

https://onions.danwin1210.de/test.php

Output: `No, the service is offline!`

### Relevant log output

_No resp
...

💬 furszy commented on pull request "test: make assumeUTXO test capture the expected fatal error":
(https://github.com/bitcoin/bitcoin/pull/28050#discussion_r1257284611)
Didn't know that the macro existed. Thanks.

> Finally, it may be good to add steps to reproduce/test for reviewers. Otherwise it is unclear what your goal is. Reading the description you are trying to avoid that some error string is output?

Yeah. The `chainstatemanager_snapshot_completion_hash_mismatch` unit test manually triggers a fatal error, and the test framework by default prints the error message to the console ([this one](https://github.com/bitcoin/bitcoin/blob/79e8247ddb166f9b980f
...

🤔 furszy reviewed a pull request: "test: make assumeUTXO test capture the expected fatal error"
(https://github.com/bitcoin/bitcoin/pull/28050#pullrequestreview-1520722077)
updated per feedback, thanks @MarcoFalke.

* Moved `DebugLogHelper` usage to `ASSERT_DEBUG_LOG`.
* Added reproduction steps to the PR description.