🤔 vasild reviewed a pull request: "net: attempts to connect to all resolved addresses when connecting to a node"
(https://github.com/bitcoin/bitcoin/pull/28834#pullrequestreview-1877328078)
Approach ACK 494c732fabf656764114bfbf824e5aa60c80e2cf, just a naming nit below.
(https://github.com/bitcoin/bitcoin/pull/28834#pullrequestreview-1877328078)
Approach ACK 494c732fabf656764114bfbf824e5aa60c80e2cf, just a naming nit below.
💬 vasild commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1487488123)
Here we iterate over all resolved addresses and I think this is the right thing to do as usually there are just a few, maybe IPv4 and IPv6 and some fallback addresses for redundancy.
What is the worst thing that can happen? A broken/malicious hostname could resolve to 100s addresses all of which timeout slowly, causing some delay here. I think that is ok, given that `pzsDest` never comes unchecked from random/evil peers over the P2P network and is always provided manually by the operator of t
...
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1487488123)
Here we iterate over all resolved addresses and I think this is the right thing to do as usually there are just a few, maybe IPv4 and IPv6 and some fallback addresses for redundancy.
What is the worst thing that can happen? A broken/malicious hostname could resolve to 100s addresses all of which timeout slowly, causing some delay here. I think that is ok, given that `pzsDest` never comes unchecked from random/evil peers over the P2P network and is always provided manually by the operator of t
...
💬 vasild commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1487480449)
naming: variable names should use snake_case according to `doc/developer-notes.md`.
Also, this contains the "resolved" addresses in the first branch, but in the other branches it has just a copy of `addrConnect`. In that case there is no "resolving" involved. Maybe a better name would be `connect_to`?
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1487480449)
naming: variable names should use snake_case according to `doc/developer-notes.md`.
Also, this contains the "resolved" addresses in the first branch, but in the other branches it has just a copy of `addrConnect`. In that case there is no "resolving" involved. Maybe a better name would be `connect_to`?
👍 BrandonOdiwuor approved a pull request: "test, assumeutxo: Add test to ensure failure when mempool not empty"
(https://github.com/bitcoin/bitcoin/pull/29394#pullrequestreview-1877373101)
ACK 8d20602e555dfe026b421363ee32cfca17c674d8
(https://github.com/bitcoin/bitcoin/pull/29394#pullrequestreview-1877373101)
ACK 8d20602e555dfe026b421363ee32cfca17c674d8
✅ Sjors closed a pull request: "rpc: add path to gethdkey"
(https://github.com/bitcoin/bitcoin/pull/22341)
(https://github.com/bitcoin/bitcoin/pull/22341)
💬 Sjors commented on pull request "rpc: add path to gethdkey":
(https://github.com/bitcoin/bitcoin/pull/22341#issuecomment-1940966185)
This seems more involved than a simple rebase. #29130 changes `gethdkey` to `gethdkeys` making it a less obvious choice to add a `path` argument to. We probably need a new RPC that's more similar to `createwalletdescriptor` in how it's called.
I probably won't have time for this in the near future, so I'm closing this as up for grabs.
(https://github.com/bitcoin/bitcoin/pull/22341#issuecomment-1940966185)
This seems more involved than a simple rebase. #29130 changes `gethdkey` to `gethdkeys` making it a less obvious choice to add a `path` argument to. We probably need a new RPC that's more similar to `createwalletdescriptor` in how it's called.
I probably won't have time for this in the near future, so I'm closing this as up for grabs.
💬 maflcko commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1487554652)
Not sure if a new test file with all the overhead (starting a node) is needed to call a single RPC. Seems easier to just call the RPC in an existing test
(https://github.com/bitcoin/bitcoin/pull/29387#discussion_r1487554652)
Not sure if a new test file with all the overhead (starting a node) is needed to call a single RPC. Seems easier to just call the RPC in an existing test
🤔 josibake reviewed a pull request: "wallet: batch erase procedures and improve 'EraseRecords' performance"
(https://github.com/bitcoin/bitcoin/pull/29403#pullrequestreview-1877500958)
code review ACK https://github.com/bitcoin/bitcoin/pull/29403/commits/77331aa2a198b708372a5c6cdf331faf7e2968ef
If you end up retouching, I think it's better to keep all of the `RunWithinTxn` changes in the first commit. Makes it easier to review and avoids introducing the function with one signature and then changing the signature in a later commit.
(https://github.com/bitcoin/bitcoin/pull/29403#pullrequestreview-1877500958)
code review ACK https://github.com/bitcoin/bitcoin/pull/29403/commits/77331aa2a198b708372a5c6cdf331faf7e2968ef
If you end up retouching, I think it's better to keep all of the `RunWithinTxn` changes in the first commit. Makes it easier to review and avoids introducing the function with one signature and then changing the signature in a later commit.
💬 josibake commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1487550243)
in "wallet: simplify EraseRecords by using 'ErasePrefix'" (https://github.com/bitcoin/bitcoin/pull/29403/commits/77331aa2a198b708372a5c6cdf331faf7e2968ef):
nit: these changes should be in the first commit
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1487550243)
in "wallet: simplify EraseRecords by using 'ErasePrefix'" (https://github.com/bitcoin/bitcoin/pull/29403/commits/77331aa2a198b708372a5c6cdf331faf7e2968ef):
nit: these changes should be in the first commit
💬 josibake commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1487551717)
in "wallet: simplify EraseRecords by using 'ErasePrefix'" (https://github.com/bitcoin/bitcoin/pull/29403/commits/77331aa2a198b708372a5c6cdf331faf7e2968ef):
nit: these changes should be in the first commit
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1487551717)
in "wallet: simplify EraseRecords by using 'ErasePrefix'" (https://github.com/bitcoin/bitcoin/pull/29403/commits/77331aa2a198b708372a5c6cdf331faf7e2968ef):
nit: these changes should be in the first commit
💬 maflcko commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-1941201466)
At the risk of having asked this previously: Why not postpone the 1c7d8be commit to a later pull? This would also make review easier, because the code change comes with the changes to actually use it in real code, outside of just unit tests.
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-1941201466)
At the risk of having asked this previously: Why not postpone the 1c7d8be commit to a later pull? This would also make review easier, because the code change comes with the changes to actually use it in real code, outside of just unit tests.
💬 hebasto commented on pull request "wallet: simplify and batch zap wallet txes process":
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1487645981)
nit: The trailing space could be easily overlooked and dropped during translation.
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1487645981)
nit: The trailing space could be easily overlooked and dropped during translation.
💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1487646437)
> The current approach, update BroadcastTransaction to support checking fee rate then introduce the new error type which in my opinion, is the better approach.
sounds good!
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1487646437)
> The current approach, update BroadcastTransaction to support checking fee rate then introduce the new error type which in my opinion, is the better approach.
sounds good!
💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1941242157)
CI failure doesn't make any sense given the lines that you changed in your latest force push. Could be a transient failure or a silent merge conflict? FWIW, I rebased locally on master and ran the functional tests without any failures.
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1941242157)
CI failure doesn't make any sense given the lines that you changed in your latest force push. Could be a transient failure or a silent merge conflict? FWIW, I rebased locally on master and ran the functional tests without any failures.
📝 hebasto opened a pull request: "qt: Update translation source file for v27.0 string freeze"
(https://github.com/bitcoin-core/gui/pull/793)
This PR updates the `src/qt/locale/bitcoin_en.xlf` translation source file according to the [Release schedule for 27.0](https://github.com/bitcoin/bitcoin/issues/29028).
Note for reviewers: it is expected to get a zero diff after running `make -C src translate` locally.
(https://github.com/bitcoin-core/gui/pull/793)
This PR updates the `src/qt/locale/bitcoin_en.xlf` translation source file according to the [Release schedule for 27.0](https://github.com/bitcoin/bitcoin/issues/29028).
Note for reviewers: it is expected to get a zero diff after running `make -C src translate` locally.
💬 hebasto commented on pull request "Update translation source file for v27.0 string freeze":
(https://github.com/bitcoin-core/gui/pull/793#issuecomment-1941259666)
Friendly ping @jarolrod @johnny9 @pablomartin4btc :)
(https://github.com/bitcoin-core/gui/pull/793#issuecomment-1941259666)
Friendly ping @jarolrod @johnny9 @pablomartin4btc :)
💬 vasild commented on pull request "addrman: delete addresses that don't belong to the supported networks":
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1941308243)
> not ensuring we're sending Tor addresses to Tor peers is also not positive
Right, if we sent solely IPv4 addresses to a Tor peer while we have Tor addresses, that seems like something that can be improved.
(https://github.com/bitcoin/bitcoin/pull/29330#issuecomment-1941308243)
> not ensuring we're sending Tor addresses to Tor peers is also not positive
Right, if we sent solely IPv4 addresses to a Tor peer while we have Tor addresses, that seems like something that can be improved.
💬 sdaftuar commented on pull request "p2p: Increase tx relay rate":
(https://github.com/bitcoin/bitcoin/pull/28592#issuecomment-1941310927)
> The drawback of having this number be too high is that we will waste our peers' bandwidth and cpu by giving them more txs than can possibly go into blocks, which they'll then have to validate and may in turn relay onwards.
Just to expand on this a bit more, a benefit of this number not being too high is that we reserve some of the network's relay capacity for higher feerate transactions that might show up after, say, a flood of low feerate transactions arrives.
If we had no limit at all
...
(https://github.com/bitcoin/bitcoin/pull/28592#issuecomment-1941310927)
> The drawback of having this number be too high is that we will waste our peers' bandwidth and cpu by giving them more txs than can possibly go into blocks, which they'll then have to validate and may in turn relay onwards.
Just to expand on this a bit more, a benefit of this number not being too high is that we reserve some of the network's relay capacity for higher feerate transactions that might show up after, say, a flood of low feerate transactions arrives.
If we had no limit at all
...
💬 furszy commented on pull request "wallet: simplify and batch zap wallet txes process":
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1487741844)
> nit: The trailing space could be easily overlooked and dropped during translation.
@hebasto, what about creating a constant to represent the whitespace, or alternatively, implementing a function that concatenates strings with the whitespace inserted between them?
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1487741844)
> nit: The trailing space could be easily overlooked and dropped during translation.
@hebasto, what about creating a constant to represent the whitespace, or alternatively, implementing a function that concatenates strings with the whitespace inserted between them?
💬 josibake commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#issuecomment-1941383895)
Concept ACK
Starting to review, if you get a chance can you rebase this so that it's only the relevant outstanding commits?
(https://github.com/bitcoin/bitcoin/pull/28574#issuecomment-1941383895)
Concept ACK
Starting to review, if you get a chance can you rebase this so that it's only the relevant outstanding commits?