💬 MarcoFalke commented on pull request "contrib: docs fix --import-keys flag on verify.py":
(https://github.com/bitcoin/bitcoin/pull/27840#issuecomment-1585615215)
lgtm ACK ceb01689351e738436393cf7b8d84cb7fafc7b98

📝 pinheadmz opened a pull request: "Add unit & functional test coverage for blockstore"
(https://github.com/bitcoin/bitcoin/pull/27850)
This PR adds unit and functional tests to cover the behavior described in #2039. In particular, that bitcoind will crash on startup if a reindex is requested but the `blk` files are read-only. Eventually this behavior can be updated with https://github.com/bitcoin/bitcoin/pull/27039. This PR just commits the test coverage from #27039 as suggested in https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1584915782

💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1585663521)

> If this is done I think we could merge everything except the last two commits in a separate PR, and start having better test coverage in master right away.

Great plan, good idea thanks. Unit and functional tests opened in separate PR: https://github.com/bitcoin/bitcoin/pull/27850 I'll return to this after the tests and other conflicting PRs are merged or closed.

⚠️ 072222 opened an issue: "https://coinmarketcap.com/currencies/bitcoin/"
(https://github.com/bitcoin/bitcoin/issues/27851)

✅ fanquake closed an issue: "https://coinmarketcap.com/currencies/bitcoin/"
(https://github.com/bitcoin/bitcoin/issues/27851)

:lock: fanquake locked an issue: "https://coinmarketcap.com/currencies/bitcoin/"
(https://github.com/bitcoin/bitcoin/issues/27851)

💬 furszy commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1225398417)
> You could fix and future-proof this by adding a interfaces::Node::setExitStatus() function, or just update the existing [appInitMain()](https://github.com/bitcoin/bitcoin/blob/153a6882f42fff3fdc63bf770d4c86a62c46c448/src/node/interfaces.cpp#L106-L109) function to set the error code.

Great, added it. Thanks!

> If you let appInitMain set the error code, you could also simplify GUI code even more, skipping Q_EMIT initializeResult(rv, tip_info) when appInitMain fails and getting rid of the r
...

💬 furszy commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1225399092)
Done, added

💬 furszy commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#issuecomment-1585690708)
Updated per feedback, thanks ryanofsky. [Tiny diff](https://github.com/bitcoin/bitcoin/compare/da60d86bfbe7fe49ce7181b5b394c0e3d8c53854..61c569ab6069d04079a0831468eb713983919636):

- Removed `BitcoinApplication::getExitStatus`.
- Moved `exit_status.store(EXIT_FAILURE)` to the interface `appInitMain()` method to be compatible with [#10102](https://github.com/bitcoin/bitcoin/pull/10102).

💬 TheCharlatan commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1585733097)
Concept ACK

💬 LarryRuane commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1585733171)
Please consider editing the description slightly, if my understanding is correct.

>With this PR, if the inbound connection is on our whitelist we start the eviction process by selecting a random unprotected peer.

At this point in the process, where we choose a random peer, we're considering _all_ (inbound, not noban) peers, is that correct? They are all (other than outbound or noban) "unprotected" because we haven't protected any of them yet. So maybe this should say, "... selecting a rand
...

👍 theStack approved a pull request: "util: implement `noexcept` move assignment & move ctor for `prevector`"
(https://github.com/bitcoin/bitcoin/pull/27334#pullrequestreview-1473456023)
Tested and light code-review ACK bfb9291a8661fe5b26c14ed755cfa89d27c37110

(My understanding of move semantics is limited, but the change looks correct to me. [All the links @jonatack posted above](https://github.com/bitcoin/bitcoin/pull/27334#pullrequestreview-1371788591) for further reading were very helpful, especially this one: https://quuxplusone.github.io/blog/2022/08/26/vector-pessimization/)

@ryanofsky: you might be interested in this PR?

💬 Giszmo commented on issue "Indefinite "Bitcoin Core is shutting down..."":
(https://github.com/bitcoin/bitcoin/issues/27848#issuecomment-1585750520)
So ... may I kill it or would you want any additional information while it's still refusing to stop on its own?

🤔 LarryRuane reviewed a pull request: "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full"
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1430899133)
utACK fa78fc543cd46ee1c7181ecf6b696c2892b9f8d3
except for possible attack vectors (for example, `NoBanForce`) that I'm not really qualified to comment on. I may try to contribute to that discussion after I've learned more. Nice PR!

💬 LarryRuane commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1196647358)
```suggestion
// Return the last erased (not-to-be-evicted) element
```

💬 LarryRuane commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1225492297)
Perhaps insert a comment before this line `// This may still return std::nullopt`

💬 LarryRuane commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1225488646)
@vasild
>This PR currently expands the semantic of the `noban` permission

Could you please elaborate on this point? I thought that with this PR, `noban` nodes are still [completely protected](https://github.com/bitcoin/bitcoin/pull/27600/files#diff-2404112ebf57bee5f9a16f1a6e1ecfc27a981d37a1c0ff202b4cd9bdfa3e48ccR183) (they are removed from the eviction candidates list before the random node is chosen). But it's certain that I'm missing something. Thanks.

💬 LarryRuane commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1225502218)
I'm not a fan of default arguments; consider making this not have a default. It makes sense if there are many existing calls to a function and you don't want to touch them all, but there's only one call to this function in production code. You would have to touch about 6 call sites in test code, but that's not too bad. The reason I'm not a fan of default arguments is that they hide something that may be helpful to see when just looking at the call site. (You might think, "Oh, it's possible that
...

💬 pinheadmz commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#issuecomment-1585761906)
concept ACK

I was looking for this writing https://github.com/bitcoin/bitcoin/pull/27850 was surprised that a fatal internal error didn't satisfy the python test for assert init error on startup

💬 dimitaracev commented on pull request "ci: Use podman stop over podman kill":
(https://github.com/bitcoin/bitcoin/pull/27844#issuecomment-1585766862)
ACK [faaa627](https://github.com/bitcoin/bitcoin/pull/27844/commits/faaa62754e84417baa917f20db379db78146687d)