💬 achow101 commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#issuecomment-1939776161)
reACK 77331aa2a198b708372a5c6cdf331faf7e2968ef
(https://github.com/bitcoin/bitcoin/pull/29403#issuecomment-1939776161)
reACK 77331aa2a198b708372a5c6cdf331faf7e2968ef
💬 theuni commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1939796003)
Replaced my commit with @TheCharlatan's artwork above.
It doesn't work in the c-i environment because `bitcoin-config.h.in` is missing. I added an `./autogen.sh` to the script but that doesn't help because that env doesn't have autoconf.
Next I tried prepending with a bare `autoheader`, which should be all that's actually needed, but that's missing too.
Sadly, I'm not sure it's worth continuing with this. Even if it does work, the verify script would have to modify the user's tree which
...
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1939796003)
Replaced my commit with @TheCharlatan's artwork above.
It doesn't work in the c-i environment because `bitcoin-config.h.in` is missing. I added an `./autogen.sh` to the script but that doesn't help because that env doesn't have autoconf.
Next I tried prepending with a bare `autoheader`, which should be all that's actually needed, but that's missing too.
Sadly, I'm not sure it's worth continuing with this. Even if it does work, the verify script would have to modify the user's tree which
...
💬 tdb3 commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-1939902686)
> > Rebased. This has failed CI once when waiting for the disconnection after:
> > ```
> > cur_mock_time += (HEADERS_RESPONSE_TIME + 1)
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > I've tried to reproduce it but I cannot in my local setup, the only thing that comes to mind is that mock time may be too close to the timeout (only one second ahead), so I gave it a delta of 10 seconds instead.
> > PS: For context, you can check the
...
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-1939902686)
> > Rebased. This has failed CI once when waiting for the disconnection after:
> > ```
> > cur_mock_time += (HEADERS_RESPONSE_TIME + 1)
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > I've tried to reproduce it but I cannot in my local setup, the only thing that comes to mind is that mock time may be too close to the timeout (only one second ahead), so I gave it a delta of 10 seconds instead.
> > PS: For context, you can check the
...
💬 TheCharlatan commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1940697823)
Re https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1939796003
Could just replace the first command then. Not as nice, but for the purpose of this PR, reviewers can run the mentioned command manually: https://github.com/TheCharlatan/bitcoin/commit/b78e3ce41168600a1e73cb079b7eba07893753c4 (also applies some fixes, because the verify script uses `sh` and not `bash` :P).
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1940697823)
Re https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1939796003
Could just replace the first command then. Not as nice, but for the purpose of this PR, reviewers can run the mentioned command manually: https://github.com/TheCharlatan/bitcoin/commit/b78e3ce41168600a1e73cb079b7eba07893753c4 (also applies some fixes, because the verify script uses `sh` and not `bash` :P).
💬 maflcko commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1487390188)
Unless there's an objection, I'd follow up with this in a follow-up?
(https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1487390188)
Unless there's an objection, I'd follow up with this in a follow-up?
💬 maflcko commented on pull request "getrawtransaction implementation":
(https://github.com/bitcoin-core/gui/pull/777#issuecomment-1940767749)
Seems confusing to have the same feature present twice. Once in this pop-up and another time in the RPC pop-up?
If this is merged, then every RPC will be duplicated in a new GUI pop-up?
(https://github.com/bitcoin-core/gui/pull/777#issuecomment-1940767749)
Seems confusing to have the same feature present twice. Once in this pop-up and another time in the RPC pop-up?
If this is merged, then every RPC will be duplicated in a new GUI pop-up?
💬 maflcko commented on issue "Bitcoin Core GUI getrawtransaction implementation":
(https://github.com/bitcoin-core/gui/issues/645#issuecomment-1940770556)
Not sure. Seems easier to just use the RPC window, with the benefit that it has more docs and doesn't need an update if the RPC changes.
(https://github.com/bitcoin-core/gui/issues/645#issuecomment-1940770556)
Not sure. Seems easier to just use the RPC window, with the benefit that it has more docs and doesn't need an update if the RPC changes.
✅ 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`?