📝 maflcko opened a pull request: "NOMERGE DEBUG WIP ignore"
(https://github.com/bitcoin/bitcoin/pull/33028)
(https://github.com/bitcoin/bitcoin/pull/33028)
💬 fanquake commented on pull request "ci: Use APT_LLVM_V in msan task":
(https://github.com/bitcoin/bitcoin/pull/32999#issuecomment-3096172593)
Should probably adjust the timeout / comment:
https://github.com/bitcoin/bitcoin/blob/5878f35446ae260ccb0ab5051b795b4364f77951/.cirrus.yml#L160
but given it's being moved in #32989, left a note there.
(https://github.com/bitcoin/bitcoin/pull/32999#issuecomment-3096172593)
Should probably adjust the timeout / comment:
https://github.com/bitcoin/bitcoin/blob/5878f35446ae260ccb0ab5051b795b4364f77951/.cirrus.yml#L160
but given it's being moved in #32989, left a note there.
💬 fanquake commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2218810639)
Note to remember to drop the timeout post #32999.
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2218810639)
Note to remember to drop the timeout post #32999.
🤔 ajtowns reviewed a pull request: "refactor: GenTxid type safety followups"
(https://github.com/bitcoin/bitcoin/pull/33005#pullrequestreview-3037443575)
Approach ACK
(https://github.com/bitcoin/bitcoin/pull/33005#pullrequestreview-3037443575)
Approach ACK
💬 ajtowns commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2218771665)
Maybe add a comment to call attention to the `const&`and the reason for it?
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2218771665)
Maybe add a comment to call attention to the `const&`and the reason for it?
💬 ajtowns commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2218799503)
Passing in the `GenTxid` and keeping the `visit` inside the function (rather than an `if` outside it along with templatizing) seems nicer?
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2218799503)
Passing in the `GenTxid` and keeping the `visit` inside the function (rather than an `if` outside it along with templatizing) seems nicer?
🚀 fanquake merged a pull request: "ci: Use APT_LLVM_V in msan task"
(https://github.com/bitcoin/bitcoin/pull/32999)
(https://github.com/bitcoin/bitcoin/pull/32999)
💬 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
...