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

👍 furszy approved a pull request: "refactor: Move stopafterblockimport option out of blockstorage"
(https://github.com/bitcoin/bitcoin/pull/28053#pullrequestreview-1520725353)
ACK 5867bb1a

💬 furszy commented on pull request "refactor: Move stopafterblockimport option out of blockstorage":
(https://github.com/bitcoin/bitcoin/pull/28053#discussion_r1257289914)
tiny nit:
A bit more readable if it uses `bool` instead of `auto`. (and also, maybe a better name for the variable would be `stop_after_import`)

💬 WaynePerth74 commented on issue "Improving fee estimation accuracy":
(https://github.com/bitcoin/bitcoin/issues/27995#issuecomment-1627397920)
free rate meed to be in union or better tet need to be unifited i would recomend dev team implament someting simulor to mempool.spoce

all so on asidenote i request dev team to reintraduce mining funchion to bitcoin core but have it mainly used for asic miners as cpu and gpu mining of bitcoin isnt safisent