💬 ryanofsky commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1224430665)
In commit "refactor: index: use `AbortNode` in fatal error helper" (3c06926cf21dcca3074ef51506f556b2286c299b)

It would be slightly better to just pass a reference to node.exit_status instead of passing the whole NodeContext, because passing NodeContext exposes a lot of other state this function doesn't need to have access to. But it's not important because #27711 gets rid of the InitShutdownState function and will drop the parameter anyway.

💬 ryanofsky commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1224433824)
In commit "gui: return EXIT_FAILURE on post-init fatal errors" (c8806e972f2be9f8a05bf11b1f6a8a2e70e01962)

Should delete the "Set exit result" comment here or move it down below. Probably fine to delete because `exit_status.store(EXIT_FAILURE)` should pretty clear by itself.

💬 ryanofsky commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1224448255)
In commit "refactor: decouple early return commands from AppInit" (61fe1af01418d8b93d38aba8aeb65aecfa6a95cd)

Note for reviewers: Moving the `common::InitConfig` call out of the try block means if InitConfig throws an exception it won't be caught. But this is good and should not change behavior because InitConfig is already catching `std::exception` internally and should just be doing basic parsing and not triggering other types of exceptions.

💬 ryanofsky commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1224438294)
In commit "gui: return EXIT_FAILURE on post-init fatal errors" (c8806e972f2be9f8a05bf11b1f6a8a2e70e01962)

This is ok for now, but if you want to future-proof the code and make it compatible with #10102, it would be better to add an `interfaces::Node::getExitStatus()` method and have GUI call `node().getExitStatus()`. The problem is that with #10102, `node().context()` will return null when the node is running in a different process than the GUI.

I also think it might be clearer just to de
...

💬 Tguntenaar commented on pull request "contrib: docs fix --import-keys flag on verify.py":
(https://github.com/bitcoin/bitcoin/pull/27840#issuecomment-1584766328)
I ran into this together with @bufo24, therefore ACK [ceb0168](https://github.com/bitcoin/bitcoin/commit/ceb01689351e738436393cf7b8d84cb7fafc7b98)

💬 hazeycode commented on issue "Stuck chainstate when utxo_snapshot.sh fails":
(https://github.com/bitcoin/bitcoin/issues/27841#issuecomment-1584805127)
Thanks @jamesob. I can confirm that `reconsiderblock` works as you expected. I'll have a go at updating the script with error handing.

📝 hazeycode opened a pull request: "script: utxo_snapshot.sh error handling"
(https://github.com/bitcoin/bitcoin/pull/27845)
There are 2 parts to this patchset, both concern the operation of `./contrib/devtools/utxo_snapshot.sh`:
1. Errors handling is added so that the chainstate is automatically recovered if an error occurs
2. Check if a file at OUTPUT_PATH already exists and early exit

The easiest way to test this is to take only the 1st commit and follow these steps:
1. Create a dummy file for the purpose of the test e.g. `echo "some bytes so file is not empty" > ./utxo-788440.dat`
2. Run bitcoind and wait f
...

💬 ryanofsky commented on pull request "Use `int32_t` type for most transaction size/weight values":
(https://github.com/bitcoin/bitcoin/pull/23962#issuecomment-1584892205)
Following patch fixes the silent merge conflict for me after a rebase:

```diff
diff --git a/src/test/miniminer_tests.cpp b/src/test/miniminer_tests.cpp
index 371147b0e23c..b26f7dafe342 100644
--- a/src/test/miniminer_tests.cpp
+++ b/src/test/miniminer_tests.cpp
@@ -137,7 +137,7 @@ BOOST_FIXTURE_TEST_CASE(miniminer_1p1c, TestChain100Setup)

std::vector<CTransactionRef> all_transactions{tx1, tx2, tx3, tx4, tx5, tx6, tx7, tx8};
struct TxDimensions {
- size_t vsize; CAm
...

💬 jamesob commented on pull request "script: utxo_snapshot.sh error handling":
(https://github.com/bitcoin/bitcoin/pull/27845#issuecomment-1584900789)
Thanks for working on this! crACK

💬 jamesob commented on pull request "script: utxo_snapshot.sh error handling":
(https://github.com/bitcoin/bitcoin/pull/27845#issuecomment-1584903028)
I see you've got some lint failures in CI; you can run linters (faithfully) locally using the container added here: https://github.com/bitcoin/bitcoin/pull/26906/files

💬 ryanofsky commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1584915782)
> We are likely going to defer mergeing this for the moment, as it conflicts with the next two kernel PRs, and assumeutxo. Up to you, if you'd like to keep it rebased after each merge, in the interim.

I agree it probably makes sense to delay the core part of this change moving the `FlushBlockFile` call. But I think the rest of the PR makes adds meaningful test coverage to parts of the code that are lacking it, and could help reassure correctness about the other PRs.

Relatedly, I think the
...

👍 ryanofsky approved a pull request: "validation: Move warningcache to ChainstateManager and rename to m_warningcache"
(https://github.com/bitcoin/bitcoin/pull/27357#pullrequestreview-1472767513)
Code review ACK 93daff4b4577a07994b57218df9bb83bed7bf743

This change does seem ready to merge, but I notice it conflicts with #27596 so might not be worth creating more rebase churn there. Will leave alone for now and wait for more input or a better opportunity to merge

🤔 theStack reviewed a pull request: "util: implement `noexcept` move assignment & move ctor for `prevector`"
(https://github.com/bitcoin/bitcoin/pull/27334#pullrequestreview-1472768128)
Concept ACK

Very nice find! Here are the benchmark results on one of my slower machines (running OpenBSD 7.3, amd64). Interestingly, the speedup on the `PrevectorFillVectorIndirect*` benchmarks gained by marking `noexcept` seems to be even higher than 2x (close to 3x):

merge-base:

| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 13,998.14 | 71,438.05 | 1
...

💬 theuni commented on issue "Use semantic analysis in lint-logs.py":
(https://github.com/bitcoin/bitcoin/issues/27825#issuecomment-1584955466)
Hah, I [implemented this as a clang-tidy check](https://github.com/theuni/bitcoin-tidy-experiments/blob/main/LogPrintfCheck.cpp) a while back.

I'll work on getting it hooked up.

💬 theuni commented on pull request "[DEMO] Integrate `bitcoin-tidy` clang-tidy plugin - warn on uninitialized members of aggregate types":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1584958812)
Nice, that one's already done :)

I'll update it and we'll start with that one instead.

💬 dimitaracev commented on pull request "validation: Move warningcache to ChainstateManager and rename to m_warningcache":
(https://github.com/bitcoin/bitcoin/pull/27357#issuecomment-1584959319)
The priority of this change should be pretty low anyways so I agree with that.

💬 stratospher commented on pull request "test: add python implementation of Elligator swift":
(https://github.com/bitcoin/bitcoin/pull/24005#issuecomment-1584966952)
@theStack I've updated the PR to include your suggestion. Documenting it like this might also help people who haven't gone through the algo notice this behaviour in the tests.

📝 Xekyo opened a pull request: "[coinselection] Increase SRD target by change_fee"
(https://github.com/bitcoin/bitcoin/pull/27846)
I discovered via fuzzing of another coin selection approach that at extremely high feerates SRD may find input sets that lead to transactions without change outputs. This is an unintended outcome since SRD is meant to always produce a transaction with a change output—we use other algorithms to specifically search for changeless solutions.

The issue occures when the flat allowance of 50,000 ṩ for change is insufficient to pay for the creation of a change output with a non-dust amount, at and a
...

💬 Xekyo commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1224612164)
I think you might get some funny behavior here, if `min_viable_change` is smaller than `m_change_fee` as mentioned above, also see https://github.com/bitcoin/bitcoin/pull/27846

👍 ryanofsky approved a pull request: "p2p: skip netgroup diversity follow-up"
(https://github.com/bitcoin/bitcoin/pull/27467#pullrequestreview-1472859378)
Code review ACK 11bb31c1c43b5da36ca8509b5747abfb3278ffcd

This change seems like an improvement to me, but I also think the early comment calling it "small and trivial" was appropriate and useful for clarifying the purpose of the change. Reviewers need to be able to express their honest opinions so limited review bandwidth goes to the right places, and so PR authors have a chance to improve and justify their PRs and not sit around wondering why a PR isn't being reviewed