💬 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

💬 jamesob commented on pull request "blockstorage: XOR blocksdir *.dat files":
(https://github.com/bitcoin/bitcoin/pull/28052#issuecomment-1627431981)
Concept ACK. Certainly worth doing.

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