👍 ryanofsky approved a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1516827895)
Code review ACK 30b2511f39d32e29f9f05859aa8a97b84c22376b. Just
`GetFirstStoredBlock` and `CheckBlockDataAvailability` related improvements since last review.

It would be nice to have a third reviewer for PR this if anyone else wants to take a look.

---

re: https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1254460179

> maybe would be good to discuss further what "pruneheight" result should be in a follow-up. it sounds better if we directly return the last pruned height.

I
...

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1254618240)
In commit "refactor: simplify pruning violation check" (9ef5099dac06f0cc4b904fefe9620c9fc09c0ddc)

Would be probably make sense to move new this comment to the earlier commit "make GetFirstStoredBlock assert that 'start_block' always has data" (79449741fa7b2fc3e59dbc92e408cff547a885fa), since everything described is true there and applies more to that commit.

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1254647941)
In commit "index: verify blocks data existence only once" (30b2511f39d32e29f9f05859aa8a97b84c22376b)

The variable name `need_start_from_scratch` seems misleading because it will be true even when not syncing from genesis. Would suggest simplifying and clarifying with:

```diff
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1892,9 +1892,6 @@ bool StartIndexBackgroundSync(NodeContext& node)

ChainstateManager& chainman = *Assert(node.chainman);
for (auto index : node.indexes) {
-
...

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1254614971)
In commit "refactor: simplify pruning violation check" (9ef5099dac06f0cc4b904fefe9620c9fc09c0ddc)

This comment is no longer true. The `CheckBlockDataAvailability` function will just return false in this case, the caller doesn't need to check if the block has data itself.

🤔 D33r-Gee reviewed a pull request: "assumeutxo (2)"
(https://github.com/bitcoin/bitcoin/pull/27596#pullrequestreview-1516971308)
Tested ACK built with and tested on WSL Ubuntu 22.04

> you'll spend a decent amount of time (~10min) loading the snapshot

this took closer to 30 min with an Intel(R) Core(TM) i9-10980HK CPU @ 2.40GHz 3.10 GHz

`getchainstates` returns:
```
{
"active_chain_type": "snapshot",
"ibd": {
"blocks": 896,
"bestblockhash": "00000000dabe35441efb615ad072e22ece58326cf0a43b990fc00aab808e8589",
"difficulty": 1,
"verificationprogress": 1.076164026301743e-06,
"snapshot
...

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1254734825)
you're right. the commit message is a relic of the past interval. will update both of these if I retouch the code

💬 ryanofsky commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#issuecomment-1624092185)
If anyone else wants to review: this PR has two acks and is a pure refactoring and I think could be merged if there is another reviewer.

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1624093362)
@willcl-ark thanks for the review!

> Slight tangent, but also slightly related to the discussion on whether IPV4 and IPV6 should be grouped... I noticed during testing that a majority of my network specific connection attempts were to IPV6 addresses, even though I don't have an IPV6 address and can't make connections to it... This appears to be because all networks are set as [Reachable by default ](https://github.com/bitcoin/bitcoin/blob/c325f0fbae2d6472a78be733d499783f8b69d6b8/src/net.h#L1
...

🤔 ryanofsky reviewed a pull request: "blockstorage: Return on fatal flush errors"
(https://github.com/bitcoin/bitcoin/pull/27866#pullrequestreview-1517040699)
Noticed in IRC libbitcoinkernel updates:

> \<achow101> it looks like the current pr is #27866, although #27861 has also been getting review

IMO, this PR could be an improvement, but right now seems like a draft change. I think the main thing that's missing is a clear description of how it changes behavior, and why the behavior change is safe.

💬 ryanofsky commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1254749159)
re: https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1227107197

> introduced granularly for each fatal error call site here: [TheCharlatan#13](https://github.com/TheCharlatan/bitcoin/pull/13).

Thanks, it's good to see nodiscard being used there, and introducing more meaningful error codes.

It's possible my suggestion and your PRs have slightly different goals. Maybe your goal is to enhance libbitcoinkernel applications ability to handle and report errors, and my goal is to impr
...

💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1254760855)
Did about 40M executions, and no issues! Gonna calculate the Waste score below here as discussed.

💬 achow101 commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#issuecomment-1624141412)
light ACK 6eb33bd0c21b3e075fbab596351cacafdc947472

I'm not particularly experienced with this area of the code, so I can't really comment on approach taken here. However mechanically, the code looks find and correct to me and seems to achieve the stated objectives.

💬 jonatack commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1624158995)
re-ACK 20b49460b35268db59f7efcb02736b0e31191a74

📝 furszy opened a pull request: "wallet: address book migration bug fixes"
(https://github.com/bitcoin/bitcoin/pull/28038)
Addressing two specific bugs encountered during the wallet migration process, related to the address book, and improves the test coverage for it.

Bug 1: Non-Cloning of External 'Send' Records
The external 'send' records were not being correctly cloned to all wallets.

Bug 2: Persistence of Empty Labels
As address book entries without associated db label records can be treated as change (the `label` field inside the `CAddressBookData` class is optional, `nullopt` labels make `CAddressBookD
...

💬 TheCharlatan commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1254840912)
> Maybe your goal is to enhance libbitcoinkernel applications ability to handle and report errors, and my goal is to improve clarify of validation code and find to potential bugs in it.

I think that hits it on the nail. In any case I updated the PR description with much of the body of the commit message to make it clearer what I am trying to achieve here and why I'd consider it an improvement.

📝 theuni opened a pull request: "wallet: don't include bdb files from our headers"
(https://github.com/bitcoin/bitcoin/pull/28039)
Only `#include` upstream bdb headers from our cpp files.

It's generally good practice to avoid including 3rd party deps in headers as otherwise they tend to sneak into new compilation units. IMO this makes for a nice cleanup.

There's a good bit of code movement here, but each commit is small and _should_ be obviously correct.

Note: in the future, the buildsystem can add the bdb include path for `bdb.cpp` and `salvage.cpp` only, rather than all wallet sources.

📝 theuni opened a pull request: "wallet: sqlite: don't include sqlite files from our headers"
(https://github.com/bitcoin/bitcoin/pull/28040)
Only `#include` upstream sqlite headers from our cpp files.

Like #28039 but simpler :)

💬 Xekyo commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#issuecomment-1624254517)
Concept ACK

📝 1proprogrammerchant opened a pull request: "Merge pull request #1 from bitcoin/master"
(https://github.com/bitcoin/bitcoin/pull/28041)
pull_1

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests t
...

✅ hebasto closed a pull request: "Merge pull request #1 from bitcoin/master"
(https://github.com/bitcoin/bitcoin/pull/28041)

📝 hebasto locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/28041)
pull_1

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests t
...