💬 achow101 commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1279730123)
In 29d9d326d5193bb9a410a8881eabc93de5dd6266 "[txorphanage] track size of stored orphans, total and by peer"

This function is unimplemented.

💬 achow101 commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1279714521)
In 543273d96e896adf5531ed961856aa0eb70cbe57 "[log] log ProcessOrphanTx() events by wtxid"

Perhaps log both txid and wtxid?

💬 achow101 commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1279743325)
In 974d864419dd98be6e32dec3ee11f5082b060b1b "[refactor] make TxPackageTracker responsible for EraseTx and AddChildrenToWorkset"

This line seems a bit unrelated to this commit as there is no pre-existing `EraseTx`.

💬 achow101 commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1279747416)
In b5ab45e595bbcedbd602b6385b83e9ffd983f216 "[p2p/refactor] make TxPackageTracker responsible for orphan resolution"

nit: indentation

👍 ryanofsky approved a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1555524219)
Code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9. Just suggested changes since the last review. There are various small things that could be followed up on, but I think this is ready for merge.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1279760417)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (768690b7ce551cd403f8e2a099372915f6022ad4)

Would be good to directly verify the snapshot block is included:

```c++
// block before the snapshot is excluded (as well as earlier blocks)
BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.count(assumed_base->pprev), 0);
// snapshot block is included (as well as later blocks)
BOOST_CHECK_EQUAL(cs2.setBlockIndexCandidates.count(assumed_base), 1);
``
...

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1279811734)
re: https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1277557586

> [0ce805b](https://github.com/bitcoin/bitcoin/commit/0ce805b632dcb98944a931f758f76f530f5ce5f2): why isn't `BLOCK_VALID_TRANSACTIONS` also alternatively `ASSUMED_VALID`?

I'm not sure what this review comment is asking. The code comment above still seems accurate to me, but it maybe it could be improved.

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1279767675)
re: https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278312526

> [768690b](https://github.com/bitcoin/bitcoin/commit/768690b7ce551cd403f8e2a099372915f6022ad4): this had me confused, but it's because the test considers a scenario where the snapshot base block has been downloaded. It could be nice to include the moment _before_ it's downloaded where this should be `0`.

This review comment does not match my understanding. The snapshot block is `assumed_base` at height 39 and it does
...

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1279727266)
re: https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278308074

> `GetSnapshotBaseBlock()` is `nullptr` so the special-case won't apply

I may be misunderstanding, but I don't think it's true that `GetSnapshotBaseBlock` is null for the background chainstate. This is a ChainstateManager method, so `GetSnapshotBaseBlock` is going to be non-null if any snapshot is loaded, and this code will try to add the snapshot block to both chainstates, even though it's not needed for the backgrou
...

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1279781965)
In commit "Fix initialization of setBlockIndexCandidates when working with multiple chainstates" (768690b7ce551cd403f8e2a099372915f6022ad4)

I think it's a little misleading to say the other chainstate contains all block except those marked assume-valid because:

- It actually does contain one block marked assumed-valid, which is the snapshot base (block 39)
- It also excludes other blocks which are not assumed-valid because they don't have more work than the tip (blocks 1-19)

I think th
...

🚀 ryanofsky merged a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746)

🤔 furszy reviewed a pull request: "wallet: Have the wallet store the key for automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1554781243)
Just started, left few comments.

💬 furszy commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1279310998)
In 3de9672a:

Could verify here that when `num_ckeys > 0`, `num_keys == 0` and viceversa. Failing early if it is not true.

💬 furszy commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1279450800)
In 97493ab8:

What `best_time` is here?
Shouldn't this be looking for the oldest one?, e.g `(best_time == 0 || best_time > desc_time)`

💬 furszy commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1279302592)
In 3de9672a:

These two variables are being set but not accessed here. Might be good to move them to the commit where they are used.

💬 furszy commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1279844391)
Instead of adding all keys, which could be a high number, could only add the ones related to xpubs.
e.g. (quick and dirty, should also do the same for the encrypted ones)
```diff
diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
--- a/src/wallet/walletdb.cpp (revision 7f44e83bb544f1e5c398b87c8eb7870eed3ed1ba)
+++ b/src/wallet/walletdb.cpp (date 1690834804005)
@@ -903,10 +903,19 @@
assert(spk_man);
spk_man->SetCache(cache);

+ // Obtain xpub
+
...

💬 BvsCapitalpay commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1659137796)
W a
On Mon, 31 Jul 2023 at 14:46, Luke rDashjr ***@***.***> wrote:

> ...the purpose of including signet in this repo is about making it easier
> to test bitcoin, not about building your own altcoin with different
> parameters. How does using a 30s signet do anything but encourage
> developers to build software that will only work well on altcoins (like
> liquid...
>
> Just thought I should point out that Liquid is Bitcoin, not an altcoin.
>
> —
> Reply to this email directly, view i
...

💬 jonatack commented on pull request "ci: Use documented `CCACHE_MAXSIZE` instead of `CCACHE_SIZE`":
(https://github.com/bitcoin/bitcoin/pull/28188#issuecomment-1659138814)
Post-merge ACK

💬 achow101 commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1279854922)
In e82f4e7d7b41592b8891a2c6f9b961b2fc2ec51e "[txpackages] add AncestorPackage for linearizing packages"

nit: avoid looking up twice

```suggestion
const auto& entry_it = m_txid_to_entry.find(tx->GetHash());
if (entry_it == m_txid_to_entry.end()) return std::nullopt;
const auto& entry = entry_it->second;
```

💬 achow101 commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1279847172)
In e82f4e7d7b41592b8891a2c6f9b961b2fc2ec51e "[txpackages] add AncestorPackage for linearizing packages"

typo

```suggestion
// (a.fee * b.vsize > b.fee * a.vsize) is a shortcut for (a.fee / a.vsize > b.fee / b.vsize)
```

💬 achow101 commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1279861059)
In e82f4e7d7b41592b8891a2c6f9b961b2fc2ec51e "[txpackages] add AncestorPackage for linearizing packages"

Since all of the transactions in the package are already stored in `PackageEntry`s in `m_txid_to_entry`, could `m_txns` just be a vector of `PackageEntry&`? This would reduce the number of lookups to `m_txid_entry` that are needed just to sort the transactions when we are returning them.

Additionally, if functions like `GetAncestorSet` used a `std::set<PackageEntry&>`, then `std::sort` a
...