💬 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?
💬 Sjors commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#issuecomment-1941392829)
Concept ACK. This removes a lot of the complexity compared to #26728.
Did a light code review of b55b46f07ae60bbd51ed9abb2f8c5b3249af60f2 which looks good, though clearly CI is unhappy.
(https://github.com/bitcoin/bitcoin/pull/29130#issuecomment-1941392829)
Concept ACK. This removes a lot of the complexity compared to #26728.
Did a light code review of b55b46f07ae60bbd51ed9abb2f8c5b3249af60f2 which looks good, though clearly CI is unhappy.
👍 Sjors approved a pull request: "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/29130#pullrequestreview-1877425600)
Concept ACK. This removes a lot of the complexity compared to #26728.
Did a light code review of b55b46f07ae60bbd51ed9abb2f8c5b3249af60f2 which looks good, though clearly CI is unhappy.
(https://github.com/bitcoin/bitcoin/pull/29130#pullrequestreview-1877425600)
Concept ACK. This removes a lot of the complexity compared to #26728.
Did a light code review of b55b46f07ae60bbd51ed9abb2f8c5b3249af60f2 which looks good, though clearly CI is unhappy.
💬 Sjors commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1487519941)
b09e9ae6a61a96d3e04619f7514a57470e220abe: nit drop `print`
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1487519941)
b09e9ae6a61a96d3e04619f7514a57470e220abe: nit drop `print`
💬 Sjors commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1487737712)
8221484b95c9e376ea6cb437eb972457485b65a7: "HD keys of non-ranged descriptors" ?
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1487737712)
8221484b95c9e376ea6cb437eb972457485b65a7: "HD keys of non-ranged descriptors" ?
💬 Sjors commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1487554071)
b55b46f07ae60bbd51ed9abb2f8c5b3249af60f2: nit: `["descs"]` is not used
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1487554071)
b55b46f07ae60bbd51ed9abb2f8c5b3249af60f2: nit: `["descs"]` is not used
💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1487757625)
Done
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1487757625)
Done
💬 Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1941426438)
Rebased to give CI another try.
HWI now returns better errors, though I have not yet tested it here.
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1941426438)
Rebased to give CI another try.
HWI now returns better errors, though I have not yet tested it here.
✅ fanquake closed an issue: "wallet_reorgrestore.py failed"
(https://github.com/bitcoin/bitcoin/issues/29392)
(https://github.com/bitcoin/bitcoin/issues/29392)
🚀 fanquake merged a pull request: "test: fix intermittent failure in wallet_reorgrestore.py"
(https://github.com/bitcoin/bitcoin/pull/29425)
(https://github.com/bitcoin/bitcoin/pull/29425)
💬 sdaftuar commented on pull request "v3 followups":
(https://github.com/bitcoin/bitcoin/pull/29424#issuecomment-1941441498)
utACK 6b161cb82a9766ef814f05e5b8f019f15d5ee14d
(https://github.com/bitcoin/bitcoin/pull/29424#issuecomment-1941441498)
utACK 6b161cb82a9766ef814f05e5b8f019f15d5ee14d