💬 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)
💬 totdking commented on issue "A missing import to the src/chainparamsbase.h":
(https://github.com/bitcoin/bitcoin/issues/33019#issuecomment-3096966958)
I added the import and it worked :) , thanks man 👍
(https://github.com/bitcoin/bitcoin/issues/33019#issuecomment-3096966958)
I added the import and it worked :) , thanks man 👍
💬 maflcko commented on pull request "docs: adds correct updated documentation links":
(https://github.com/bitcoin/bitcoin/pull/32699#issuecomment-3096984862)
No, you haven't replied/addressed the feedback from June: https://github.com/bitcoin/bitcoin/pull/32699#discussion_r2150261914
(https://github.com/bitcoin/bitcoin/pull/32699#issuecomment-3096984862)
No, you haven't replied/addressed the feedback from June: https://github.com/bitcoin/bitcoin/pull/32699#discussion_r2150261914
✅ pinheadmz closed an issue: "b-msghand invoked oom-killer in Debug build: Master (v28.99) crashing during IBD"
(https://github.com/bitcoin/bitcoin/issues/31561)
(https://github.com/bitcoin/bitcoin/issues/31561)
💬 pinheadmz commented on issue "b-msghand invoked oom-killer in Debug build: Master (v28.99) crashing during IBD":
(https://github.com/bitcoin/bitcoin/issues/31561#issuecomment-3096984988)
Closing this now, `Bitcoin Core daemon version v29.99.0-c43cc48aaaaa (debug build)` synced succesfully on a debian VM 8GB memory and no swap
(https://github.com/bitcoin/bitcoin/issues/31561#issuecomment-3096984988)
Closing this now, `Bitcoin Core daemon version v29.99.0-c43cc48aaaaa (debug build)` synced succesfully on a debian VM 8GB memory and no swap