💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2218854573)
Unrelatedly, the `USE_CIRRUS_RUNNERS` can probably be removed:
* It is only there for forks, but the comment says it doesn't work for forks anyway in the expected way.
* If someone maintains a fork (presumably to add patches), it should be trivial for them to make the required patch to the CI as well.
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2218854573)
Unrelatedly, the `USE_CIRRUS_RUNNERS` can probably be removed:
* It is only there for forks, but the comment says it doesn't work for forks anyway in the expected way.
* If someone maintains a fork (presumably to add patches), it should be trivial for them to make the required patch to the CI as well.
🚀 fanquake merged a pull request: "test: fix `ReadTopologicalSet` unsigned integer overflow"
(https://github.com/bitcoin/bitcoin/pull/33007)
(https://github.com/bitcoin/bitcoin/pull/33007)
💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#issuecomment-3096237139)
Rebased after the merge of #32521 to pick up recent changes to `AreInputsStandard`.
A new verbose message will be returned when the BIP54 legacy sigops limit is violated:
* `bad-txns-non-witness-sigops-exceeds-bip54-limit`: This occurs when the total number of non-witness sigops across the entire transaction exceeds `MAX_TX_LEGACY_SIGOPS`. Additionally, a debug message is also returned with the actual number of sigops and the limit, for example: `sigops 2501 > limit 2500`.
[3cde7f1852
...
(https://github.com/bitcoin/bitcoin/pull/29060#issuecomment-3096237139)
Rebased after the merge of #32521 to pick up recent changes to `AreInputsStandard`.
A new verbose message will be returned when the BIP54 legacy sigops limit is violated:
* `bad-txns-non-witness-sigops-exceeds-bip54-limit`: This occurs when the total number of non-witness sigops across the entire transaction exceeds `MAX_TX_LEGACY_SIGOPS`. Additionally, a debug message is also returned with the actual number of sigops and the limit, for example: `sigops 2501 > limit 2500`.
[3cde7f1852
...
🤔 ismaelsadeeq reviewed a pull request: "mempool: Avoid needless vtx iteration during IBD"
(https://github.com/bitcoin/bitcoin/pull/32827#pullrequestreview-3037622642)
reACK 249889bee6b88eb9814eb969e6fb108f86a4bf98
(https://github.com/bitcoin/bitcoin/pull/32827#pullrequestreview-3037622642)
reACK 249889bee6b88eb9814eb969e6fb108f86a4bf98
💬 ismaelsadeeq commented on pull request "mempool: Avoid needless vtx iteration during IBD":
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2218883957)
In "mempool: Avoid expensive loop in removeForBlock during IBD" 41ad2be4340d7b18a4669aa9becef8936597f000
nit: change commit title to "mempool: avoid looping `blk` `txs` when mempool is empty in `removeForBlock`"
(https://github.com/bitcoin/bitcoin/pull/32827#discussion_r2218883957)
In "mempool: Avoid expensive loop in removeForBlock during IBD" 41ad2be4340d7b18a4669aa9becef8936597f000
nit: change commit title to "mempool: avoid looping `blk` `txs` when mempool is empty in `removeForBlock`"
💬 maflcko commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2218901137)
nit in https://github.com/bitcoin/bitcoin/commit/ecc6c07e583842a2afe1a8b5bb8ec0bb9f997fdb: Could add and use a small helper: `TxoutType GetTxoutType(const CScript& output_script)`? This makes the code shorter below and wraps this dummy.
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2218901137)
nit in https://github.com/bitcoin/bitcoin/commit/ecc6c07e583842a2afe1a8b5bb8ec0bb9f997fdb: Could add and use a small helper: `TxoutType GetTxoutType(const CScript& output_script)`? This makes the code shorter below and wraps this dummy.
💬 maflcko commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2218546827)
nit in the first commit: could be using `using` for new code?
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2218546827)
nit in the first commit: could be using `using` for new code?
💬 maflcko commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2218582070)
nit in the first commit: Should this be called `script_base_size` instead? There are other uses of prevector (`CompressedScript`, netaddr, ...), so defining this globally could be confusing.
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2218582070)
nit in the first commit: Should this be called `script_base_size` instead? There are other uses of prevector (`CompressedScript`, netaddr, ...), so defining this globally could be confusing.
💬 maflcko commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2218877265)
nit in ecc6c07e583842a2afe1a8b5bb8ec0bb9f997fdb: Use snake_case for new code?
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2218877265)
nit in ecc6c07e583842a2afe1a8b5bb8ec0bb9f997fdb: Use snake_case for new code?
💬 rkrux commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2218957649)
In 2551fdfdd662dfb4077c3059b437acf56bdbaf5b "wallet/migration: use count private key count to identify watchonly descs"
This evaluation of `watchonly` at a barebones level looks great to me! Though maybe we can put this inside a `IsWatchOnly` function in the Descriptor{Impl} class? Reading all these details in the migration flow might be distracting, and moreover this generic function might have its own use cases separate from this later. Following patch builds and tests fine on my system. I
...
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2218957649)
In 2551fdfdd662dfb4077c3059b437acf56bdbaf5b "wallet/migration: use count private key count to identify watchonly descs"
This evaluation of `watchonly` at a barebones level looks great to me! Though maybe we can put this inside a `IsWatchOnly` function in the Descriptor{Impl} class? Reading all these details in the migration flow might be distracting, and moreover this generic function might have its own use cases separate from this later. Following patch builds and tests fine on my system. I
...
💬 rkrux commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2218970582)
Sorry but I don't understand how the second statement follows the first one above.
For a non-watch-only wallet, isn't it guaranteed that there must be at least one private key, which should turn `has_priv_key` to `true`?
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2218970582)
Sorry but I don't understand how the second statement follows the first one above.
For a non-watch-only wallet, isn't it guaranteed that there must be at least one private key, which should turn `has_priv_key` to `true`?
💬 zaidmstrr commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-3096459508)
The method `rpcRunLater` was removed in [commit](https://github.com/bitcoin/bitcoin/commit/fcfd3db563e89fd79820a4cdfa102d624d801de1#diff-8e9fef4d0718d085f31da382899d97e3bbefc8d6a72ede95916a0e2fbd1a532f), thus there are some build errors.
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-3096459508)
The method `rpcRunLater` was removed in [commit](https://github.com/bitcoin/bitcoin/commit/fcfd3db563e89fd79820a4cdfa102d624d801de1#diff-8e9fef4d0718d085f31da382899d97e3bbefc8d6a72ede95916a0e2fbd1a532f), thus there are some build errors.
💬 Sjors commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2219082048)
> The silent payments wallet needs to scan entire blocks since it doesn't know ahead of time which scriptPubKeys to use in a GCS filter.
This makes sense. Is it documented anywhere so we don't forget by the next review round? :-)
> It might also be possible to use fast rescan when importing descriptors if none of the descriptors are silent payment descriptors.
It wasn't referring to that and I wouldn't worry about this. Somewhere in the RPC documentation (createwallet and/or importdescr
...
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2219082048)
> The silent payments wallet needs to scan entire blocks since it doesn't know ahead of time which scriptPubKeys to use in a GCS filter.
This makes sense. Is it documented anywhere so we don't forget by the next review round? :-)
> It might also be possible to use fast rescan when importing descriptors if none of the descriptors are silent payment descriptors.
It wasn't referring to that and I wouldn't worry about this. Somewhere in the RPC documentation (createwallet and/or importdescr
...
💬 danielabrozzoni commented on pull request "p2p: rename GetAddresses -> GetAddressesUncached":
(https://github.com/bitcoin/bitcoin/pull/32994#discussion_r2219096854)
I also don't love the `GetAddresses`/`GetAddressesUncached` naming either, but I agree with stickies that it's important that the unsafe version stands out.
What if we instead renamed them to `GetAddressesCached` / `GetAddressesUncached`? That way, there’s no ambiguity about what each function does, and the naming feels a bit more consistent.
(https://github.com/bitcoin/bitcoin/pull/32994#discussion_r2219096854)
I also don't love the `GetAddresses`/`GetAddressesUncached` naming either, but I agree with stickies that it's important that the unsafe version stands out.
What if we instead renamed them to `GetAddressesCached` / `GetAddressesUncached`? That way, there’s no ambiguity about what each function does, and the naming feels a bit more consistent.
💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2219135907)
> This makes sense. Is it documented anywhere so we don't forget by the next review round? :-)
I added a separate commit that disables fast rescan and I added an explanation to the commit message. Will push soon.
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2219135907)
> This makes sense. Is it documented anywhere so we don't forget by the next review round? :-)
I added a separate commit that disables fast rescan and I added an explanation to the commit message. Will push soon.
💬 m3dwards commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2219202190)
Done
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2219202190)
Done
💬 maflcko commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2219238228)
implementat -> implement [spelling error in comment “…able to implementat a ReadField…”]
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2219238228)
implementat -> implement [spelling error in comment “…able to implementat a ReadField…”]
💬 darosior commented on pull request "script: return verification flag responsible for error upon validation failure":
(https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3096821342)
The behaviour already exists in a limited fashion. Even without this change, an attacker can trivially create such a transaction (as discussed in OP). Here is for instance a `p2p_invalid_tx.py` case which passes both with or without this PR:
<details>
<summary> Click to see patch </summary>
```diff
diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py
index 36b274efc27..46bb2bba367 100644
--- a/test/functional/data/invalid_txs.py
+++ b/test/functiona
...
(https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3096821342)
The behaviour already exists in a limited fashion. Even without this change, an attacker can trivially create such a transaction (as discussed in OP). Here is for instance a `p2p_invalid_tx.py` case which passes both with or without this PR:
<details>
<summary> Click to see patch </summary>
```diff
diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py
index 36b274efc27..46bb2bba367 100644
--- a/test/functional/data/invalid_txs.py
+++ b/test/functiona
...
💬 marcofleon commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2219264339)
Fair, it does feel cleaner using GenTxid and avoiding the branch in `ProcessGetData`. I chose to templatize in response to this [comment](https://github.com/bitcoin/bitcoin/pull/32631#pullrequestreview-2877628308) from the original PR. The templating takes care of everything at compile-time, which is nice. But `m_most_recent_block_txs` holds GenTxids, so in the templated version we're creating a temporary GenTxid anyway for that map lookup vs creating a GenTxid once upfront.
I think it's larg
...
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2219264339)
Fair, it does feel cleaner using GenTxid and avoiding the branch in `ProcessGetData`. I chose to templatize in response to this [comment](https://github.com/bitcoin/bitcoin/pull/32631#pullrequestreview-2877628308) from the original PR. The templating takes care of everything at compile-time, which is nice. But `m_most_recent_block_txs` holds GenTxids, so in the templated version we're creating a temporary GenTxid anyway for that map lookup vs creating a GenTxid once upfront.
I think it's larg
...
💬 danielabrozzoni commented on pull request "p2p: rename GetAddresses -> GetAddressesUncached":
(https://github.com/bitcoin/bitcoin/pull/32994#issuecomment-3096922403)
In my last push (df65b527f00cbf9fd6675bf22bb1285bfce24f66):
- split changes into two commits (one for rename, one for docs)
(https://github.com/bitcoin/bitcoin/pull/32994#issuecomment-3096922403)
In my last push (df65b527f00cbf9fd6675bf22bb1285bfce24f66):
- split changes into two commits (one for rename, one for docs)