📝 ryanofsky opened a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048)
This change drops the last kernel dependency on shutdown.cpp. It also adds new hooks for libbitcoinkernel applications to be able to stop kernel operations after blocks are imported and each time the chain tip changes.

💬 hebasto commented on pull request "build: Add CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/25797#issuecomment-1625710276)
Closing this PR as its usefulness seems marginal.

Although, I'll keep maintaining this branch.

Reminding that the active reviewing process still happening in https://github.com/hebasto/bitcoin/tree/cmake-staging and related PRs.

💬 ryanofsky commented on pull request "kernel: Remove shutdown globals from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1256159653)
re: https://github.com/bitcoin/bitcoin/pull/27711#discussion_r1214568318

> I think it would be great to get rid of the 3 StartShutdown calls in kernel code, but probably better to do in a separate PR that was independent of this one, since there's lots of approaches that could be used and none of them seem to depend on the other changes here.

I implemented this idea in #28048, getting rid of the `stop_after_block_import` and `stop_at_height` kernel options and replacing them with more flex
...

✅ hebasto closed a pull request: "build: Add CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/25797)

💬 achow101 commented on pull request "wallet: don't include bdb files from our headers":
(https://github.com/bitcoin/bitcoin/pull/28039#issuecomment-1625712726)
reACK 8b5397c00e821d7eaab22f512e9d71a1a0392ebf

💬 TheCharlatan commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#issuecomment-1625724128)
Rebased 1fe06a811bf8be941bef5336af97688f48885828 -> d1c92b57a72b62ffa202f1d3d357b59befdc9c12 ([kernelReturnOnAbort_0](https://github.com/TheCharlatan/bitcoin/tree/kernelReturnOnAbort_0) -> [kernelReturnOnAbort_1](https://github.com/TheCharlatan/bitcoin/tree/kernelReturnOnAbort_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelReturnOnAbort_0..kernelReturnOnAbort_1))

* Fixed conflict with #27861
* Fixed formatting if formatting

💬 luke-jr commented on pull request "build: Add CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/25797#issuecomment-1625724201)
>we should be able to enumerate some more benefits to doing so.

One more for the list:

* Real out-of-tree builds. Autotools requires autogen to be run in the tree, which can be complicated when you don't want the build to have write access to the source dir.

🚀 achow101 merged a pull request: "wallet: don't include bdb files from our headers"
(https://github.com/bitcoin/bitcoin/pull/28039)

💬 santochibtc commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1625747312)
Thanks @sipa, yes, it is "a histogram of the fee rates paid by transactions in the memory pool, weighted by transaction size", but my point is still valid. If you want to include your tx in the next block you must estimate your fee based on the pending txs that are paying more per byte. I think that the histogram is not always a good picture of the mempool to estimate fees.

💬 ryanofsky commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#issuecomment-1625782988)
Updated 828af9ad19a702e711963f9b498f7462bde8aabb -> c2bcc339d7b8def58289d3ff01b2a905dc291ed6 ([`pr/stopafter.1`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.1) -> [`pr/stopafter.2`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/stopafter.1..pr/stopafter.2)) just updating comments and adding release note about `-stopatheight` behavior

🤔 ishaanam reviewed a pull request: "wallet: Deniability API (Unilateral Transaction Meta-Privacy)"
(https://github.com/bitcoin/bitcoin/pull/27792#pullrequestreview-1519449241)
Hey @denavila, welcome to Bitcoin Core and thanks for working on this! While I think that this idea could be useful to some extent if implemented correctly, I have a few concerns about the current implementation:
- Currently there is no precaution against a user spending two outputs of a deniabilization transaction together. IMO I think that this is quite problematic and could lead to these deniabilization transactions being rendered ineffective. Not only that, but then it also becomes obvious
...

💬 theuni commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#issuecomment-1625907483)
Very nice. Concept ACK and quick (very shallow) codereview ACK.

I think the `-stopatheight` change is reasonable. If it turns out anyone/anything was relying on the way this worked before, we could always add functionality for that use-case (maybe `-stopatblock`?)

However, the help string should be updated from:

> Stop running after reaching the given height in the main chain

It sounds like it's not completely accurate now, and less so after this change :)

💬 achow101 commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1625915605)
Assuming it doesn't break a whole lot of tests, I would prefer to change it always use the new version as it removes the need to check that everything is using the right variant.

💬 jonatack commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#issuecomment-1625936888)
Thank you for the reviews @TheCharlatan and @MarcoFalke. Updated the name of the new helper and removed the static annotations per https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1250420188, should be trivial to re-ACK.

💬 theuni commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1256350258)
Is it possible for this to be called twice?
```c++
if (m_stop_at_height) {
if (index.nHeight > m_stop_at_height) {
// assert(false) or return kernel::Interrupted{} ?
} else if (index.nHeight == m_stop_at_height) {
..
}
}
```

Not that I imagine calling `StartShutdown()` twice would be particularly harmful anyway.

⚠️ dsldsl opened an issue: "Malware reported in scan of bitcoin-qt versions 22 and 24.0.1 apple darwin"
(https://github.com/bitcoin/bitcoin/issues/28049)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

<img width="1174" alt="image" src="https://github.com/bitcoin/bitcoin/assets/50015/625e57b0-6a86-486d-b18a-58c93ce60f26">

My virus scanner reported the malware Koiot found in the dmg file and in the installed app of bitcoin core. Is this a known false positive or an actual potential issue with a bad binary I downloaded? thank you.

### Expected behaviour

Not have a virus

### Steps t
...

🤔 ryanofsky reviewed a pull request: "blockstorage: Return on fatal flush errors"
(https://github.com/bitcoin/bitcoin/pull/27866#pullrequestreview-1519541703)
Code review d1c92b57a72b62ffa202f1d3d357b59befdc9c12. I think the extra error reporting here is great, and some of the changes in behavior seem good too. But I think there is too much happening in one commit. There are 3-4 separate changes in behavior that could be implemented in separate commits, and I think would be more clearly understood and evaluated that way.

💬 ryanofsky commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1256329428)
In commit "blockstorage: Return on fatal flush errors" (d1c92b57a72b62ffa202f1d3d357b59befdc9c12)

This skips trying to flush the undo file if flushing the block file fails. It think it would be safer and more conservative to keep previous behavior of flushing both files in sequence.

You can still return false if either flush call fails, but I don't see a benefit in changing behavior here and potentially making code less robust, when it's not necessary just to add a return code.

I think
...

💬 ryanofsky commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1256284494)
In commit "blockstorage: Return on fatal flush errors" (d1c92b57a72b62ffa202f1d3d357b59befdc9c12)

Let me know if I understand this correctly: The new `return false` avoids writing block data to a new block file if the _previous_ block file is full and cannot be synced and trimmed. This seems to be safe because this `FindBlockPos` function only has one caller, `SaveBlockToDisk`, which only has two callers, `Chainstate::AcceptBlock` and `Chainstate::LoadGenesisBlock` which both seem to handle t
...

💬 ryanofsky commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1256350394)
In commit "blockstorage: Return on fatal flush errors" (d1c92b57a72b62ffa202f1d3d357b59befdc9c12)

This change seems fragile to me, and it's unclear what the benefits are. I think it would be great to add a log print here, but if this this code is going to be changed to return false, I think that should happen in a separate followup commit, not combined with other changes here, and have a clear explanation and justification.

IIUC, this change introduces an inconsistency. For all block data,
...