💬 mzumsande commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1244359758)
commit 68b13a2b6c96fd061a31add58b8626c5bf1cdf3d:
I think that once it's decoupled from `BaseIndex::Init()`, the names `BaseIndex::Start()` and `StartIndexes()` are misleading: It doesn't actually start the indexes, if `Init()` determines that an index is synced, the index is started right then (it begins to process validationinterface signals), and the later `StartIndexes()` does nothing, and could just as well be skipped. I'd prefer something like `IndexBackgroundSync()`.

🤔 mzumsande reviewed a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1501321408)
Concept ACK, didn't review the latest push yet. Only have some issues with the naming.

💬 mzumsande commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1244340856)
commit 11fb8fa81ab8e2436e4ae6de79b52581db399265:
This spot doesn't mean the `ThreadImport` function, but the thread. Various other spots below also mean the thread, so I think it'd be better to change this manually, and to also expand the description one line below? I think essentially this thread performs various loading tasks that are part of init but shouldn't block the node from being started, so are done in a separate thread (external block import, reindex, reindex-chainstate, index backg
...

💬 mzumsande commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1244362593)
commit 68b13a2b6c96fd061a31add58b8626c5bf1cdf3d:
Considering how large this function becomes and how low-level the code is in the final state, I don't really love having it in `init.cpp` instead of somewhere in `index/`. Could it be better in `index/base.cpp` as a free function, even if it's not part of `BaseIndex`?

💬 TheCharlatan commented on pull request "kernel: Add interrupt class":
(https://github.com/bitcoin/bitcoin/pull/27861#issuecomment-1610260838)
Updated e0653c2223f8d6d74645c9b5742de1c069d8c3a7 -> 0c2fd16275ae078e33d71b0c9da8034075998553 ([kernelInterrupt_6](https://github.com/TheCharlatan/bitcoin/commits/kernelInterrupt_6) -> [kernelInterrupt_7](https://github.com/TheCharlatan/bitcoin/commits/kernelInterrupt_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelInterrupt_6..kernelInterrupt_7)).
* Adjusted and simplified documentation as suggested by @ryanofsky

Rebased 0c2fd16275ae078e33d71b0c9da8034075998553 -> 001acf
...

💬 achow101 commented on pull request "Remove the syscall sandbox":
(https://github.com/bitcoin/bitcoin/pull/27896#issuecomment-1610264637)
ACK 32e2ffc39374f61bb2435da507f285459985df9e

The syscall sandbox has a rather significant maintenance burden for rather limited benefit.

✅ achow101 closed an issue: "Outcome of the syscall sandbox experiment"
(https://github.com/bitcoin/bitcoin/issues/24771)

🚀 achow101 merged a pull request: "Remove the syscall sandbox"
(https://github.com/bitcoin/bitcoin/pull/27896)

👍 ryanofsky approved a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1501951643)
Code review ACK 3c83b1d884b419adece95b335b6e956e7459a7ef. Just Marco's suggested error handling fixes since last review

🤔 jonatack reviewed a pull request: "test: add end-to-end tests for CConnman and PeerManager"
(https://github.com/bitcoin/bitcoin/pull/26812#pullrequestreview-1501951235)
Reviewed/built/tested the first four commits up to https://github.com/bitcoin/bitcoin/commit/d2f46c705540c74c2b6f83a66535c3ead1cb95d4.

💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1244454225)
d2f46c7 Verified the behavior improvement in this commit by changing this line to make it fail.

master

```bash
$ ./src/test/test_bitcoin -t banman_tests
Running 1 test case...
libc++abi: terminating due to uncaught exception of type std::runtime_error: 'Dropping entry with unknown version (2) rom ban list' not found in debug log

******** errors disabling the alternate stack:
#error:22
Invalid argument
unknown location:0: fatal error: in "banman_tests/file": signal: SIGABR
...

💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1244442031)
Rather than including everything, it might be good to have a separate compilation unit for code that needs to be in `util/setup_common`, versus the current `util/net` code that is only needed for a handful of the tests.

🚀 ryanofsky merged a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914)

👍 ryanofsky approved a pull request: "kernel: Add interrupt class"
(https://github.com/bitcoin/bitcoin/pull/27861#pullrequestreview-1502027676)
Code review ACK 001acf37dff40688d82c3b1a7ff9ed3901addb33. This looks very good, and hopefully can be merged soon with more review.

To encourage review, I would maybe rewrite the PR title and description to be a little less abstract, and describe the benefits concretely. For title, would suggest "kernel: Remove all `ShutdownRequested` and `AbortNode` calls from validation code." For description would suggest: "Replace all `AbortNode` calls in validation code with new fatal error and flush erro
...

💬 ryanofsky commented on pull request "Disable and uncheck blank when private keys are disabled":
(https://github.com/bitcoin-core/gui/pull/739#issuecomment-1610420201)
@S3RK do you think the UI changes here are actually worse, or just not an improvement? I agree the changes aren't ideal and aren't a clear win, but I think they don't make it worse necessarily. This PR disables the "make blank wallet" checkbox when "disable private keys" checkbox is enabled, instead of just automatically checking it. I think disabling the checkbox makes sense because it makes it clear than once private keys are disabled, the choice of whether to have a blank wallet is moot becau
...

💬 miketwenty1 commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1610431814)
@Sjors Not sure if you saw my response above, let me know what you think, we can close this out if there's no appetite for this small change.

💬 evansmj commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1244653007)
What do you think of renaming the `TxStateConflicted` struct to `TxStateBlockConflicted`, since that one is used for block conflicts and there is now a difference here between block and mempool conflicts?

`//! State of rejected transaction that conflicts with a confirmed block.
struct TxStateConflicted {
...
};`

👍 stratospher approved a pull request: "Introduce secp256k1 module with field and group classes to test framework"
(https://github.com/bitcoin/bitcoin/pull/26222#pullrequestreview-1502244056)
tested ACK d4fb58a. really liked how this PR makes the secp256k1 code in the tests more intuitive and easier to follow!

✅ MarcoFalke closed an issue: "An option for a shell command that runs just before bitcoind completes shutting down."
(https://github.com/bitcoin/bitcoin/issues/27984)

💬 hebasto commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1610800583)
Someone on Twitter drew attention to potential security implications, for instance:
- [New ZIP domains spark debate among cybersecurity experts](https://www.bleepingcomputer.com/news/security/new-zip-domains-spark-debate-among-cybersecurity-experts/)
- [Don't Click That ZIP File! Phishers Weaponizing .ZIP Domains to Trick Victims](https://thehackernews.com/2023/05/dont-click-that-zip-file-phishers.html)

💬 MarcoFalke commented on issue "Add maxrelaytxfee":
(https://github.com/bitcoin/bitcoin/issues/27983#issuecomment-1610811473)
I don't understand this. If the inputs are signed by someone other than the miner, they may (or should) have broadcast the transaction already, making this setting useless. Also, once it is included in a block, the transaction can be fee-sniped by another miner in a re-org.
And obviously implementing the setting as suggested by you will also apply it to network transactions, which doesn't make sense either.