💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1253461532)
```suggestion
LogPrint(BCLog::TXPACKAGES, "\nForgetting orphan request %s from peer=%d\n", gtxid.GetHash().ToString(), nodeid);
```

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1253619584)
since we're using `size_t` for these fields, do we want to continue on with UB for release builds or do an assert?

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1253479136)
trying to fetch after failure seems wrong

```suggestion
m_orphanage.EraseTx(wtxid);
orphan_request_tracker.ForgetTxHash(wtxid);
```

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1253623061)
> [txorphanage] handle AddTx(nullptr)

Could we motivate this change in the commit message?

💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1253643847)
Suggested comment, something like:

"We do not presume the parent will still be in the orphanage by the time a response is received, so we exclude the orphanage from the check when deciding what to request."

If that's wrong, then it needs better explanation than what exists in the commit message :)

💬 RandyMcMillan commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1623991042)
Concept ACK

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