💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1487461162)
Fixed
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1487461162)
Fixed
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1487461569)
Thanks updated
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1487461569)
Thanks updated
💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1487481307)
The `MAX_FEERATE_EXCEEDED` error type was not introduced yet at this point.
This is correct because at https://github.com/bitcoin/bitcoin/commit/eedbeccbdb274841fc2cf2c53931e58fbea50dfc, the error message indicates that the failure might be from `-maxtxfee` or `maxfeerate`.
And this is fixed in next commit 0ad2f2c32460fef9b8cd59e3920fcfd6602b02a4 after we introduced the `MAX_FEERATE_EXCEEDED` error type.
The options are;
- Introduce the error type first, then update` BroadcastTransactio
...
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1487481307)
The `MAX_FEERATE_EXCEEDED` error type was not introduced yet at this point.
This is correct because at https://github.com/bitcoin/bitcoin/commit/eedbeccbdb274841fc2cf2c53931e58fbea50dfc, the error message indicates that the failure might be from `-maxtxfee` or `maxfeerate`.
And this is fixed in next commit 0ad2f2c32460fef9b8cd59e3920fcfd6602b02a4 after we introduced the `MAX_FEERATE_EXCEEDED` error type.
The options are;
- Introduce the error type first, then update` BroadcastTransactio
...
🤔 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.