💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229667232)
I've pulled in your suggestion here.
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229667232)
I've pulled in your suggestion here.
💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229670245)
Not sure if it makes a measurable difference in performance, but I'm happy to use your version. Updated.
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229670245)
Not sure if it makes a measurable difference in performance, but I'm happy to use your version. Updated.
💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-3115124995)
Updated per review feedback (thanks!)
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-3115124995)
Updated per review feedback (thanks!)
💬 achow101 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-3115210492)
Also seeing @mzumsande's observation if I add the addresses to loopback, but not if I add them to a different interface.
I think it's reasonable to filter out all loopback addresses, so the resolution is probably to just update the instructions to have the addresses bound to a non-loopback interface.
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3115210492)
Also seeing @mzumsande's observation if I add the addresses to loopback, but not if I add them to a different interface.
I think it's reasonable to filter out all loopback addresses, so the resolution is probably to just update the instructions to have the addresses bound to a non-loopback interface.
💬 monlovesmango commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3115217031)
My feedback is mostly a lot of annoying nits mainly from leftover variables and comments still referring to old max announcements DoS scoring. I compiled them in my own [branch](https://github.com/monlovesmango/bitcoin/tree/pr32941-TxOrphanage-revamp-cleanups) so you can just cherry pick whatever you think is actually worth taking. Overview below.
- [doc: Fixes GetChildrenFromSamePeer comments](https://github.com/monlovesmango/bitcoin/commit/2a25c28aec3773bdf9a17148e45ce38ee95a7342) - Updates
...
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3115217031)
My feedback is mostly a lot of annoying nits mainly from leftover variables and comments still referring to old max announcements DoS scoring. I compiled them in my own [branch](https://github.com/monlovesmango/bitcoin/tree/pr32941-TxOrphanage-revamp-cleanups) so you can just cherry pick whatever you think is actually worth taking. Overview below.
- [doc: Fixes GetChildrenFromSamePeer comments](https://github.com/monlovesmango/bitcoin/commit/2a25c28aec3773bdf9a17148e45ce38ee95a7342) - Updates
...
💬 pinheadmz 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-3115236229)
If @b-l-u-e is going to retouch to update the instructions in the test comment I'd also really like to see those instructions executed in a CI test as well.
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3115236229)
If @b-l-u-e is going to retouch to update the instructions in the test comment I'd also really like to see those instructions executed in a CI test as well.
💬 mzumsande 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-3115248007)
For me it would also ok to leave the resolution of #31336 for a separate PR - but this PR shouldn't claim to solve the issue then.
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3115248007)
For me it would also ok to leave the resolution of #31336 for a separate PR - but this PR shouldn't claim to solve the issue then.
📝 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
...