⚠️ Crypt-iQ opened an issue: "ThreadI2PAcceptIncoming temporarily bypasses 125 connection ceiling"
(https://github.com/bitcoin/bitcoin/issues/27843)
- The i2p accept code is in `ThreadI2PAcceptIncoming`, instead of in `ThreadSocketHandler` like the non-i2p accept code.
- When accepting non-i2p connections, a connection is accepted in `AcceptConnection` and if there are more than `maxInbound` outstanding connections, one is marked for disconnect. The next iteration of the loop in `ThreadSocketHandler` will call `DisconnectNodes` and remove it.
- Since the i2p code resides in its own thread, it's possible for i2p connections to stack up pas
...

📝 MarcoFalke opened a pull request: "ci: Use podman stop over podman kill"
(https://github.com/bitcoin/bitcoin/pull/27844)
This should avoid a race where the kill is not done when spinning up the new container. podman stop waits 10 seconds by default.

Fixes https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1217942753

👍 ryanofsky approved a pull request: "Return EXIT_FAILURE on post-init fatal errors"
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1472293444)
Code review ACK 61fe1af01418d8b93d38aba8aeb65aecfa6a95cd. This looks very good.

Appreciate you cleaning up the code to make it more obvious when the new `exit_status` value will and won't be returned. (And sorry I posted another buggy diff that forgot about the early returns!) Left a few more minor suggestions that you could take or leave.

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

Would be nice to just pass `ArgsManager&` to this function instead of `NodeContext& node`, since accessing other parts of the context is not needed.

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