💬 rkrux commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-3116306876)
Strong Concept ACK e6ac2015a852caa0f50e44becf9cd4b7592c48e7
Very useful for the users, the GUI option (in the separate PR) makes creating a watch-only version even easier.
I will review this PR soon.
I believe the following is done now and can be removed from the PR description?
> Some of the first several commits can be split into separate PRs if so desired. Those are primarily bug fixes for existing issues that came up during testing.
(https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-3116306876)
Strong Concept ACK e6ac2015a852caa0f50e44becf9cd4b7592c48e7
Very useful for the users, the GUI option (in the separate PR) makes creating a watch-only version even easier.
I will review this PR soon.
I believe the following is done now and can be removed from the PR description?
> Some of the first several commits can be split into separate PRs if so desired. Those are primarily bug fixes for existing issues that came up during testing.
💬 ajtowns commented on pull request "net, validation: don't punish peers for consensus-invalid txs":
(https://github.com/bitcoin/bitcoin/pull/33050#issuecomment-3116333758)
> Could you expand on the logic of this - is it just because the most effective DoS vectors known today happen to involve non-standard but valid transactions, or does this hold with more generality?
> What if the current DoS vectors involving non-standard txs are fixed in the future, and the most-effective known DoS vectors then will involve consensus-invalid txs - wouldn't we want to go back to punishing peers then?
A DoS attack needs two features: each instance needs to be relatively high
...
(https://github.com/bitcoin/bitcoin/pull/33050#issuecomment-3116333758)
> Could you expand on the logic of this - is it just because the most effective DoS vectors known today happen to involve non-standard but valid transactions, or does this hold with more generality?
> What if the current DoS vectors involving non-standard txs are fixed in the future, and the most-effective known DoS vectors then will involve consensus-invalid txs - wouldn't we want to go back to punishing peers then?
A DoS attack needs two features: each instance needs to be relatively high
...
💬 ajtowns commented on pull request "net, validation: don't punish peers for consensus-invalid txs":
(https://github.com/bitcoin/bitcoin/pull/33050#discussion_r2230103371)
Maybe "mempool-script-verify-flag-failed" and "block-script-verify-flag-failed" would be better? I didn't want to change that here since it's a bit more intrusive for no functional change. Could probably be a scripted-diff in a followup?
(https://github.com/bitcoin/bitcoin/pull/33050#discussion_r2230103371)
Maybe "mempool-script-verify-flag-failed" and "block-script-verify-flag-failed" would be better? I didn't want to change that here since it's a bit more intrusive for no functional change. Could probably be a scripted-diff in a followup?
💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3116367414)
@ryanofsky
I missed the essence of this commit, and thanks to @Sjors for clarifying that it’s intended for test coverage. If I understand correctly, test coverage is usually done on the test network.
I have no strong objection to the commit itself, just that initially I thought it demonstrate mining and I saw some edge-cases that are not covered.
It would be better to limit the mining to `regtest` only (since its for test), add a bound to the number of tries, with the recent comments, I
...
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3116367414)
@ryanofsky
I missed the essence of this commit, and thanks to @Sjors for clarifying that it’s intended for test coverage. If I understand correctly, test coverage is usually done on the test network.
I have no strong objection to the commit itself, just that initially I thought it demonstrate mining and I saw some edge-cases that are not covered.
It would be better to limit the mining to `regtest` only (since its for test), add a bound to the number of tries, with the recent comments, I
...
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-3116401175)
> I believe the following is done now and can be removed from the PR description?
Yes, removed.
(https://github.com/bitcoin/bitcoin/pull/32489#issuecomment-3116401175)
> I believe the following is done now and can be removed from the PR description?
Yes, removed.
💬 ajtowns commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-3116411732)
Still working on this? Rebase is just https://github.com/ajtowns/bitcoin/commit/d6055bdc77552f395be1a4f90b8b98c0bec5a0af
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-3116411732)
Still working on this? Rebase is just https://github.com/ajtowns/bitcoin/commit/d6055bdc77552f395be1a4f90b8b98c0bec5a0af
💬 b-l-u-e commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3116435254)
Thanks for the thorough reviews! I've investigated the issues you raised and found the root cause.
### 🔍 **Root Cause Analysis**
The test was failing because the **instructions were wrong**. The original instructions told users to use loopback interfaces (`lo:0`, `lo:1`), but the code filters out loopback addresses (line 340 in `netif.cpp`).
This is exactly what issue #31336 reported - users following the instructions got empty local addresses.
### 📝 **Reviewer Feedback Addressed**
...
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3116435254)
Thanks for the thorough reviews! I've investigated the issues you raised and found the root cause.
### 🔍 **Root Cause Analysis**
The test was failing because the **instructions were wrong**. The original instructions told users to use loopback interfaces (`lo:0`, `lo:1`), but the code filters out loopback addresses (line 340 in `netif.cpp`).
This is exactly what issue #31336 reported - users following the instructions got empty local addresses.
### 📝 **Reviewer Feedback Addressed**
...
💬 l0rinc commented on pull request "refactor: inline constant return values from `dbwrapper` write methods":
(https://github.com/bitcoin/bitcoin/pull/33042#issuecomment-3116482500)
> How verbose would it be to pass it up and check it everywhere?
With `CDBWrapper::Read` all callers are trying to handle the failure (usually by returning `false` or `std::nullopt` or `std::vector<uint256>()` or resetting the state or manually logging a failure).
I have tried unifying `Write` and `Read` via:
```diff
diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
--- a/src/node/blockstorage.h (revision 09f004bd9fecbee2216ffb6b7bb787d9ec4f0362)
+++ b/src/node/blockstorage.
...
(https://github.com/bitcoin/bitcoin/pull/33042#issuecomment-3116482500)
> How verbose would it be to pass it up and check it everywhere?
With `CDBWrapper::Read` all callers are trying to handle the failure (usually by returning `false` or `std::nullopt` or `std::vector<uint256>()` or resetting the state or manually logging a failure).
I have tried unifying `Write` and `Read` via:
```diff
diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
--- a/src/node/blockstorage.h (revision 09f004bd9fecbee2216ffb6b7bb787d9ec4f0362)
+++ b/src/node/blockstorage.
...
👍 rkrux approved a pull request: "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase"
(https://github.com/bitcoin/bitcoin/pull/33032#pullrequestreview-3054176313)
tACK 1cdb9161774ccf3edcdcb1e482b20be430ed5430
Thanks for incorporating suggestions.
(https://github.com/bitcoin/bitcoin/pull/33032#pullrequestreview-3054176313)
tACK 1cdb9161774ccf3edcdcb1e482b20be430ed5430
Thanks for incorporating suggestions.
💬 rkrux commented on pull request "wallet, test: Replace MockableDatabase with in-memory SQLiteDatabase":
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2230208195)
Nit to use tighter function signature that goes along with the function name too. Builds & tests fine on my system.
<details>
<summary>Use MockableSQLiteDatabase in return type that passes for WalletDatabase</summary>
```diff
diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp
index 6017582395..58fafeecf7 100644
--- a/src/wallet/test/util.cpp
+++ b/src/wallet/test/util.cpp
@@ -80,12 +80,12 @@ void TestUnloadWallet(std::shared_ptr<CWallet>&& wallet)
WaitForDeleteWal
...
(https://github.com/bitcoin/bitcoin/pull/33032#discussion_r2230208195)
Nit to use tighter function signature that goes along with the function name too. Builds & tests fine on my system.
<details>
<summary>Use MockableSQLiteDatabase in return type that passes for WalletDatabase</summary>
```diff
diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp
index 6017582395..58fafeecf7 100644
--- a/src/wallet/test/util.cpp
+++ b/src/wallet/test/util.cpp
@@ -80,12 +80,12 @@ void TestUnloadWallet(std::shared_ptr<CWallet>&& wallet)
WaitForDeleteWal
...
💬 maflcko commented on pull request "refactor: inline constant return values from `dbwrapper` write methods":
(https://github.com/bitcoin/bitcoin/pull/33042#issuecomment-3116540946)
> and handling all of the calls, but I'm getting failures all over the place, mostly because `Read` cannot tell the difference between missing values and errors. I'll investigate further.
Yeah, I'd presume you'd have to use `Result<bool>`, which I'd suspect would be highly verbose.
> I didn't see a single case where we're trying to handle any exception - seems we assumed the return value reflects the status.
I tried that yesterday, and an error in the index code (`scheduler` thread, or
...
(https://github.com/bitcoin/bitcoin/pull/33042#issuecomment-3116540946)
> and handling all of the calls, but I'm getting failures all over the place, mostly because `Read` cannot tell the difference between missing values and errors. I'll investigate further.
Yeah, I'd presume you'd have to use `Result<bool>`, which I'd suspect would be highly verbose.
> I didn't see a single case where we're trying to handle any exception - seems we assumed the return value reflects the status.
I tried that yesterday, and an error in the index code (`scheduler` thread, or
...
💬 maflcko commented on pull request "fuzz: Make process_message(s) more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3116570133)
fuzz-only pull with two acks and no conflicts with reviewed pulls, rfm?
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3116570133)
fuzz-only pull with two acks and no conflicts with reviewed pulls, rfm?
💬 pablomartin4btc commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2230275124)
out of curiosity... why do we need this here now? what's the issue if we don't (currently in `master`)? could we not use `UpgradeDescriptorCache()` from an upper level?
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2230275124)
out of curiosity... why do we need this here now? what's the issue if we don't (currently in `master`)? could we not use `UpgradeDescriptorCache()` from an upper level?
💬 Sjors commented on pull request "[POC] wallet: Enable non-electronic (paper-based) wallet backup with codex32":
(https://github.com/bitcoin/bitcoin/pull/33043#issuecomment-3116588223)
It might be useful to open an issue to discuss paper backups in general.
I think conceptually the are three different things to backup, and each has different requirements and frequencies:
1. Private key material: backed up once at wallet creation time (or never, in some multisig setups where recovery consists of using the other keys and moving to a fresh wallet). Needs to be kept secure against theft.
2. Output descriptors: for anything more complicated than a [BIP48 multisig](https://
...
(https://github.com/bitcoin/bitcoin/pull/33043#issuecomment-3116588223)
It might be useful to open an issue to discuss paper backups in general.
I think conceptually the are three different things to backup, and each has different requirements and frequencies:
1. Private key material: backed up once at wallet creation time (or never, in some multisig setups where recovery consists of using the other keys and moving to a fresh wallet). Needs to be kept secure against theft.
2. Output descriptors: for anything more complicated than a [BIP48 multisig](https://
...
💬 pablomartin4btc commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2230279837)
nit: shouldn't be something like GetDescriptorsInfo... ? or better naming... it's not doing an "export"...
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2230279837)
nit: shouldn't be something like GetDescriptorsInfo... ? or better naming... it's not doing an "export"...
🤔 pablomartin4btc reviewed a pull request: "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet"
(https://github.com/bitcoin/bitcoin/pull/32489#pullrequestreview-3054273346)
light cr ACK e6ac2015a852caa0f50e44becf9cd4b7592c48e7
I like the refactoring in moving the logic of extracting descriptors info out of `listdescriptors` RPC to `CWallet` itself.
I'll continue reviewing and will test it soon.
Left a couple of comments.
(https://github.com/bitcoin/bitcoin/pull/32489#pullrequestreview-3054273346)
light cr ACK e6ac2015a852caa0f50e44becf9cd4b7592c48e7
I like the refactoring in moving the logic of extracting descriptors info out of `listdescriptors` RPC to `CWallet` itself.
I'll continue reviewing and will test it soon.
Left a couple of comments.
💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2230294157)
Let's not (re)build a full fletched CPU miner :-)
(https://github.com/bitcoin/bitcoin/pull/30437#discussion_r2230294157)
Let's not (re)build a full fletched CPU miner :-)
💬 pablomartin4btc commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2230304020)
nit: this could be just GetDescriptors? you can list them or export them later?
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2230304020)
nit: this could be just GetDescriptors? you can list them or export them later?
💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3116624817)
re-utACK 3d099384a60c8f64c8d67a9f1e7b8649a9c54313
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3116624817)
re-utACK 3d099384a60c8f64c8d67a9f1e7b8649a9c54313
💬 Sjors commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2230329744)
I'm regretting that assumption in the context of multisig :-)
(https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2230329744)
I'm regretting that assumption in the context of multisig :-)