💬 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

💬 ChrisCho-H commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1659290755)
updated PR description

💬 ariard commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#discussion_r1279979090)
I think a) there is no certainty ChatGPT / LLaMa will be the most popular framework 12 / 18 months from now and I don’t think we’re going to update contributing rules everytime and b) Meta is a registered trademark of a commercial entity and I think it’s better to not give the appearance Bitcoin Core the project is supportive or supported or linked to Meta in anyway.

💬 ariard commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1659356787)
I had the feedback from the Bitcoin Defense Legal Fund, they need more time to analyze the issue though it is thought as an important one.

I did additionally cc Andrew Chow and Fanquake as maintainers on the mail thread for open-source transparency.

I think whatever one individual political opinion on copyrights or legal risks tolerance, the fact is we have already dubious copyrights litigations affecting the project, so I think it’s reasonable to wait for risk clarification before to do a
...

💬 MarcoFalke commented on pull request "test, script: python E721 and flake8 updates":
(https://github.com/bitcoin/bitcoin/pull/28194#issuecomment-1659613454)
lgtm ACK bee2d57a655645dbfaf0242e85c5af034023a2fb

💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1659618783)
Approach NACK, (but Concept ACK). Leaving a NACK only to have DrahtBot create an anchor to this comment, to avoid GitHub from hiding it as well.

I don't think you've seen https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1653599387 and GitHub is already hiding it by default.

💬 MarcoFalke commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1279295208)
```suggestion
// Ensure sensitive relay connections only send the above message types. Others are not needed and may degrade privacy.
```

(nit), to avoid having to touch this line again in the future, if something changes.