💬 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?

💬 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?

💬 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.

✅ maflcko closed an issue: "How to get started with Contribution"
(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
...

💬 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

💬 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

💬 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

💬 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
...

💬 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.

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(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

💬 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
...

🤔 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.

💬 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
...

💬 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`?

👍 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

✅ Sjors closed a pull request: "rpc: add path to gethdkey"
(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.

💬 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