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

💬 furszy commented on issue "Unsafe reduce_output when new coins are added":
(https://github.com/bitcoin/bitcoin/issues/28180#issuecomment-1659172967)
> Should we disable the adding of new inputs when reduce_output is set? If someone really intends to do that, they should probably use outputs.

I don't think so. This is more an implementation issue rather than a missing doc or a missing restriction.
This could also happen without someone intending to do it. For instance, could be a higher bump fee that make the tx require another input.

The issue arises because the code (as is now) mixes two different concepts; it treats the change outpu
...

🤔 jonatack reviewed a pull request: "script: throw disabled err for op_ver and its variants"
(https://github.com/bitcoin/bitcoin/pull/28169#pullrequestreview-1555801363)
Approach ACK ffc32938aa962f7c4b8d5f4af69bd710c174ef44

The unit script tests (`./src/test/test_bitcoin -t script_tests`) don't pass at the first commit, so it may be best to combine the second commit updating the tests into the first commit with the code changes.

💬 ChrisCho-H commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1659270516)
squashed into single commit ceab6bf3ac6b09ae7f210c05614d2c898ef7ee37

👍 theStack approved a pull request: "test: Drop 22.x node from TxindexCompatibilityTest"
(https://github.com/bitcoin/bitcoin/pull/28070#pullrequestreview-1555894628)
Code-review ACK fafe43cb6c76a5f60194be128a40baf161d39920

Additional replacement suggestion for the scripted-diff (last commit), to also tackle chain_path + "blocks" concatenations that are done via os.path.join:
`sed -i 's|].chain_path, "blocks"|].blocks_path|g' $(git grep -l chain_path)`

💬 jonatack commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1659278229)
ACK ceab6bf3ac6b09ae7f210c05614d2c898ef7ee37

- It looks like the first sentence of the PR description needs to be updated.
- Would it be good to accompany this change with a release note?

💬 darosior commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#issuecomment-1659280674)
Rebased after #27888.

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1659282959)
re-ACK 8f6eeaab38