📝 murchandamus opened a pull request: "Slay BnB Mutants"
(https://github.com/bitcoin/bitcoin/pull/33060)
I tested all of the reported surviving mutants that @brunoerg reported in https://gist.github.com/brunoerg/834063398d5002f738506d741513e310.
I found that all Mutants except for 12, 14, 17, 37, and 39 were now being caught by one of the existing tests. This fixes Mutants 14, 37, and 39.
Mutant 17 is not fixed, because I consider it acceptable that running BnB for 100,001 instead of 100,000 comparisons doesn’t cause an issue, and Mutant 12 is not yet fixed, because at `fee` = `long_term_fee
...
(https://github.com/bitcoin/bitcoin/pull/33060)
I tested all of the reported surviving mutants that @brunoerg reported in https://gist.github.com/brunoerg/834063398d5002f738506d741513e310.
I found that all Mutants except for 12, 14, 17, 37, and 39 were now being caught by one of the existing tests. This fixes Mutants 14, 37, and 39.
Mutant 17 is not fixed, because I consider it acceptable that running BnB for 100,001 instead of 100,000 comparisons doesn’t cause an issue, and Mutant 12 is not yet fixed, because at `fee` = `long_term_fee
...
💬 l0rinc commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229773767)
It's not necessarily about performance, rather just doing less work
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229773767)
It's not necessarily about performance, rather just doing less work
💬 l0rinc commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229766850)
This should likely be an `std::string` instead
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229766850)
This should likely be an `std::string` instead
💬 l0rinc commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229779981)
nit: the log sounds a bit awkward
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229779981)
nit: the log sounds a bit awkward
👍 theStack approved a pull request: "p2p: rename GetAddresses -> GetAddressesUnsafe"
(https://github.com/bitcoin/bitcoin/pull/32994#pullrequestreview-3053633002)
Code-review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
Nice documentation improvements, happy to see the method rename for improved clarity (fwiw, I'm generally not a huge fan of function overloading and think we tend to overuse it; it's often just confusing and makes it harder to find call-sites for a specific function, especially across multiple commits, e.g. for investigating past changes via `git log -S`).
(https://github.com/bitcoin/bitcoin/pull/32994#pullrequestreview-3053633002)
Code-review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
Nice documentation improvements, happy to see the method rename for improved clarity (fwiw, I'm generally not a huge fan of function overloading and think we tend to overuse it; it's often just confusing and makes it harder to find call-sites for a specific function, especially across multiple commits, e.g. for investigating past changes via `git log -S`).
📝 achow101 converted_to_draft a pull request: "Slay BnB Mutants"
(https://github.com/bitcoin/bitcoin/pull/33060)
I tested all of the reported surviving mutants that @brunoerg reported in https://gist.github.com/brunoerg/834063398d5002f738506d741513e310.
I found that all Mutants except for 12, 14, 17, 37, and 39 were now being caught by one of the existing tests. This fixes Mutants 14, 37, and 39.
Mutant 17 is not fixed, because I consider it acceptable that running BnB for 100,001 instead of 100,000 comparisons doesn’t cause an issue, and Mutant 12 is not yet fixed, because at `fee` = `long_term_fee
...
(https://github.com/bitcoin/bitcoin/pull/33060)
I tested all of the reported surviving mutants that @brunoerg reported in https://gist.github.com/brunoerg/834063398d5002f738506d741513e310.
I found that all Mutants except for 12, 14, 17, 37, and 39 were now being caught by one of the existing tests. This fixes Mutants 14, 37, and 39.
Mutant 17 is not fixed, because I consider it acceptable that running BnB for 100,001 instead of 100,000 comparisons doesn’t cause an issue, and Mutant 12 is not yet fixed, because at `fee` = `long_term_fee
...
💬 w0xlt commented on pull request "Slay BnB Mutants":
(https://github.com/bitcoin/bitcoin/pull/33060#issuecomment-3115323386)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33060#issuecomment-3115323386)
Concept ACK
💬 ismaelsadeeq commented on pull request "fees: rpc: `estimatesmartfee` now returns a fee rate estimate during low network activity":
(https://github.com/bitcoin/bitcoin/pull/32395#discussion_r2229962274)
Thanks for review.
> Why require anything at all here?
You check because you don't want to mislead the user. Imagine just finishing IBD or restarting a node after a long shutdown and then finishing the connection to the tip.
You don't want to instantly start reporting a 1 sat/vB fee rate. It has to ensure that there is indeed no activity in the network.
> What's the difference really between seeing 1 tx in the last n blocks and seeing none?
Seeing a transaction in the last n block
...
(https://github.com/bitcoin/bitcoin/pull/32395#discussion_r2229962274)
Thanks for review.
> Why require anything at all here?
You check because you don't want to mislead the user. Imagine just finishing IBD or restarting a node after a long shutdown and then finishing the connection to the tip.
You don't want to instantly start reporting a 1 sat/vB fee rate. It has to ensure that there is indeed no activity in the network.
> What's the difference really between seeing 1 tx in the last n blocks and seeing none?
Seeing a transaction in the last n block
...
💬 Eunovo commented on pull request "Silent Payments: sending":
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r2230027031)
Makes sense
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r2230027031)
Makes sense
💬 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
...