✅ maflcko closed an issue: "How to get started with Contribution"
(https://github.com/bitcoin-core/gui/issues/791)
(https://github.com/bitcoin-core/gui/issues/791)
💬 maflcko commented on issue "How to get started with Contribution":
(https://github.com/bitcoin-core/gui/issues/791#issuecomment-1940775403)
## Getting started to contribute to Bitcoin Core
### Setting up your development environment
New developers are very welcome and needed. There are a lot of open issues of any difficulty waiting to be fixed. However, before you start contributing, familiarize yourself with the Bitcoin Core build system and tests. Refer to the documentation in the repository on how to build Bitcoin Core and how to run the unit and functional tests. Once that is done, you are all set.
If you need more help
...
(https://github.com/bitcoin-core/gui/issues/791#issuecomment-1940775403)
## Getting started to contribute to Bitcoin Core
### Setting up your development environment
New developers are very welcome and needed. There are a lot of open issues of any difficulty waiting to be fixed. However, before you start contributing, familiarize yourself with the Bitcoin Core build system and tests. Refer to the documentation in the repository on how to build Bitcoin Core and how to run the unit and functional tests. Once that is done, you are all set.
If you need more help
...
💬 maflcko commented on issue "About logo icon doesn't denote chain type (?) in About/ Help message window/ dialog":
(https://github.com/bitcoin-core/gui/issues/761#issuecomment-1940779341)
Not sure. See https://github.com/bitcoin-core/gui/pull/762#issuecomment-1938757000
(https://github.com/bitcoin-core/gui/issues/761#issuecomment-1940779341)
Not sure. See https://github.com/bitcoin-core/gui/pull/762#issuecomment-1938757000
💬 maflcko commented on pull request "test: fix intermittent failure in wallet_reorgrestore.py":
(https://github.com/bitcoin/bitcoin/pull/29425#issuecomment-1940813658)
Thanks!
lgtm ACK 44d11532f80705b790bc6e28df9a96ac54b25f9b
(https://github.com/bitcoin/bitcoin/pull/29425#issuecomment-1940813658)
Thanks!
lgtm ACK 44d11532f80705b790bc6e28df9a96ac54b25f9b
💬 maflcko commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#issuecomment-1940826793)
Please don't invalidate two ACKs for a single space style issue. If you care about the style, it is your responsibility to take care of before opening the pull request, or before pushing the code. You can use any auto-formatter of your choice.
re-ACK 8d20602e555dfe026b421363ee32cfca17c674d8
(https://github.com/bitcoin/bitcoin/pull/29394#issuecomment-1940826793)
Please don't invalidate two ACKs for a single space style issue. If you care about the style, it is your responsibility to take care of before opening the pull request, or before pushing the code. You can use any auto-formatter of your choice.
re-ACK 8d20602e555dfe026b421363ee32cfca17c674d8
💬 maflcko commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1940863159)
Not sure about rushing this. I tested yesterday a bdb testnet3 wallet with 1000 txs on spinning storage, and I aborted the migration after 20 minutes. Happy to re-test if more pulls are merged.
I think the migration performance can be consider a bugfix, so I'd suggest to at least give this another two weeks after feature freeze (yesterday), according to https://github.com/bitcoin/bitcoin/issues/29028.
It seems plausible that the users having a bdb wallet with many transactions are also the
...
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1940863159)
Not sure about rushing this. I tested yesterday a bdb testnet3 wallet with 1000 txs on spinning storage, and I aborted the migration after 20 minutes. Happy to re-test if more pulls are merged.
I think the migration performance can be consider a bugfix, so I'd suggest to at least give this another two weeks after feature freeze (yesterday), according to https://github.com/bitcoin/bitcoin/issues/29028.
It seems plausible that the users having a bdb wallet with many transactions are also the
...
💬 TheCharlatan commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-1940866241)
I think there is a silent merge conflict on the first commit.
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-1940866241)
I think there is a silent merge conflict on the first commit.
💬 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