💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194151784)
I think this will unnecessarily repeat the entire path, could maybe drop `from %4$s`, I got this message just now:
> Data directory "/Users/matthewzipkin/Library/Application Support/Bitcoin/whoa" contains a "bitcoin.conf" file which is ignored, because a different configuration file "/Users/matthewzipkin/Library/Application Support/Bitcoin/bitcoin.conf" from data directory "/Users/matthewzipkin/Library/Application Support/Bitcoin" is being used instead.
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194151784)
I think this will unnecessarily repeat the entire path, could maybe drop `from %4$s`, I got this message just now:
> Data directory "/Users/matthewzipkin/Library/Application Support/Bitcoin/whoa" contains a "bitcoin.conf" file which is ignored, because a different configuration file "/Users/matthewzipkin/Library/Application Support/Bitcoin/bitcoin.conf" from data directory "/Users/matthewzipkin/Library/Application Support/Bitcoin" is being used instead.
💬 pinheadmz commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194040447)
```suggestion
- `bitcoind` and `bitcoin-qt` will now raise an error on startup if a datadir that is being used contains a bitcoin.conf file that will be ignored, which can happen when a datadir= line is used in a bitcoin.conf file. The error message is just a diagnostic intended to prevent accidental misconfiguration, and it can be disabled to restore the previous behavior of using the datadir while ignoring the bitcoin.conf contained in it.
```
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1194040447)
```suggestion
- `bitcoind` and `bitcoin-qt` will now raise an error on startup if a datadir that is being used contains a bitcoin.conf file that will be ignored, which can happen when a datadir= line is used in a bitcoin.conf file. The error message is just a diagnostic intended to prevent accidental misconfiguration, and it can be disabled to restore the previous behavior of using the datadir while ignoring the bitcoin.conf contained in it.
```
💬 furszy commented on issue ""Create Unsigned" should not show the message: "The amount exceeds you balance" without suggesting alternatives":
(https://github.com/bitcoin/bitcoin/issues/27659#issuecomment-1548438693)
> > So, probably the `importdescriptors` RPC command call was done with `timestamp=now` and no rescan was performed.
>
> I confirm that there was the `importdescriptors` RPC command call done with `timestamp=now` and no rescan was performed.
Based on that, maybe close this issue?
I think that adding the "Do you want to start rescanning Bitcoin blockchain in order to update the balance?" inside the sending process is an overkill (it will affect other scenarios and not only this one), lea
...
(https://github.com/bitcoin/bitcoin/issues/27659#issuecomment-1548438693)
> > So, probably the `importdescriptors` RPC command call was done with `timestamp=now` and no rescan was performed.
>
> I confirm that there was the `importdescriptors` RPC command call done with `timestamp=now` and no rescan was performed.
Based on that, maybe close this issue?
I think that adding the "Do you want to start rescanning Bitcoin blockchain in order to update the balance?" inside the sending process is an overkill (it will affect other scenarios and not only this one), lea
...
📝 brunoerg opened a pull request: "docs: fix spelling errors"
(https://github.com/bitcoin/bitcoin/pull/27664)
Add `lief` to `spelling.ignore-words` since it's the name of a Python lib and fix spelling error in `interface_usdt_utxocache` (s/eariler/earlier)
(https://github.com/bitcoin/bitcoin/pull/27664)
Add `lief` to `spelling.ignore-words` since it's the name of a Python lib and fix spelling error in `interface_usdt_utxocache` (s/eariler/earlier)
💬 pinheadmz commented on pull request "test: add unit test coverage for Python ECDSA implementation":
(https://github.com/bitcoin/bitcoin/pull/27653#discussion_r1194287587)
IIUC we don't assert that the keys we expect to be invalid are marked correctly. Seems like we mine as well ensure that `generate_privkey()` is always valid but certain edge cases are not (`0`, `SECP256K1_ORDER`, and `2**256 - 1`)
(https://github.com/bitcoin/bitcoin/pull/27653#discussion_r1194287587)
IIUC we don't assert that the keys we expect to be invalid are marked correctly. Seems like we mine as well ensure that `generate_privkey()` is always valid but certain edge cases are not (`0`, `SECP256K1_ORDER`, and `2**256 - 1`)
🤔 pinheadmz reviewed a pull request: "test: add unit test coverage for Python ECDSA implementation"
(https://github.com/bitcoin/bitcoin/pull/27653#pullrequestreview-1427256339)
concept ACK
I also think it would be cool to see a set of ECDSA test vectors here. We already have a set of BIP340 vectors tested both in python and in the `key_tests.cpp` unit test, assuring that both implementations handle Schnorr (at least those cases) identically.
(https://github.com/bitcoin/bitcoin/pull/27653#pullrequestreview-1427256339)
concept ACK
I also think it would be cool to see a set of ECDSA test vectors here. We already have a set of BIP340 vectors tested both in python and in the `key_tests.cpp` unit test, assuring that both implementations handle Schnorr (at least those cases) identically.
💬 sdaftuar commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1548505174)
> My understanding is that `REPLACED` would also need to be provided to peers: [#27625 (comment)](https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544171255). Otherwise there may be another round trip if the miner mines a previous version of a recently replaced transaction, which is in `mapRelay` and `vExtraTxnForCompact`, but not the mempool (and not yet in a block, because it may be in-flight)?
I think it's better that we not relay transactions that are `REPLACED` -- note that we
...
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1548505174)
> My understanding is that `REPLACED` would also need to be provided to peers: [#27625 (comment)](https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1544171255). Otherwise there may be another round trip if the miner mines a previous version of a recently replaced transaction, which is in `mapRelay` and `vExtraTxnForCompact`, but not the mempool (and not yet in a block, because it may be in-flight)?
I think it's better that we not relay transactions that are `REPLACED` -- note that we
...
💬 mzumsande commented on pull request "p2p, rpc: Manual block-relay-only connections with addnode":
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1194318233)
`m_node_address` is a string, which is part of AddedNodeParams, which has just the unvalidated user input.
`resolvedAddress` is a `CService` built from `m_node_address`, which is generated in `GetAddedNodeInfo` and used in `ThreadOpenConnection` / rpc code.
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1194318233)
`m_node_address` is a string, which is part of AddedNodeParams, which has just the unvalidated user input.
`resolvedAddress` is a `CService` built from `m_node_address`, which is generated in `GetAddedNodeInfo` and used in `ThreadOpenConnection` / rpc code.
💬 joostjager commented on pull request "rpc: allow submitpackage to be called outside of regtest":
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548517395)
There is a bugfix in here, because I think it fixes the submitpackage acceptance policy on regtest by restricting it to a safe subset of topologies? Users may use regtest now to prepare for usage of the rpc on mainnet later.
(https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1548517395)
There is a bugfix in here, because I think it fixes the submitpackage acceptance policy on regtest by restricting it to a safe subset of topologies? Users may use regtest now to prepare for usage of the rpc on mainnet later.
📝 achow101 opened a pull request: "walletdb: Remove unused CreateMockWalletDatabase"
(https://github.com/bitcoin/bitcoin/pull/27665)
This has been superseded by the MockableDatabase. Remove to avoid confusion as to which type of mock database to use for testing.
I thought this was included in #26715, maybe it got lost in a rebase.
(https://github.com/bitcoin/bitcoin/pull/27665)
This has been superseded by the MockableDatabase. Remove to avoid confusion as to which type of mock database to use for testing.
I thought this was included in #26715, maybe it got lost in a rebase.
👍 furszy approved a pull request: "walletdb: Remove unused CreateMockWalletDatabase"
(https://github.com/bitcoin/bitcoin/pull/27665#pullrequestreview-1427332880)
utACK 0282b212
(https://github.com/bitcoin/bitcoin/pull/27665#pullrequestreview-1427332880)
utACK 0282b212
📝 achow101 opened a pull request: "wallet, bench: Move commonly used functions to their own file and fix a bug"
(https://github.com/bitcoin/bitcoin/pull/27666)
I have a few PRs and branches that use these two commits, probably makes sense to split them into a separate PR to be merged sooner.
The first commit contains some things that end up being commonly used in new wallet benchmarks. These are moved into `wallet_common.{h/cpp}`.
The second commit contains a bugfix for the wallet_balance benchmark where it calls `LoadWallet` in the wrong place. It's unnecessary to call that function in this benchmark. Although this does not cause any issues curr
...
(https://github.com/bitcoin/bitcoin/pull/27666)
I have a few PRs and branches that use these two commits, probably makes sense to split them into a separate PR to be merged sooner.
The first commit contains some things that end up being commonly used in new wallet benchmarks. These are moved into `wallet_common.{h/cpp}`.
The second commit contains a bugfix for the wallet_balance benchmark where it calls `LoadWallet` in the wrong place. It's unnecessary to call that function in this benchmark. Although this does not cause any issues curr
...
💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1548545937)
First 2 commits split into #27666. Marking as draft for now.
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1548545937)
First 2 commits split into #27666. Marking as draft for now.
📝 achow101 converted_to_draft a pull request: "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/26008)
Wallets that have a ton of non-ranged descriptors (such as a migrated non-HD wallet) perform fairly poorly due to looping through all of the wallet's `ScriptPubKeyMan`s. This is done in various places, such as `IsMine`, and helper functions for fetching a `ScriptPubKeyMan` and a `SolvingProvider`. This also has a bit of a performance impact on standard descriptor wallets, although less noticeable due to the small number of SPKMs.
As these functions are based on doing `IsMine` for each `Script
...
(https://github.com/bitcoin/bitcoin/pull/26008)
Wallets that have a ton of non-ranged descriptors (such as a migrated non-HD wallet) perform fairly poorly due to looping through all of the wallet's `ScriptPubKeyMan`s. This is done in various places, such as `IsMine`, and helper functions for fetching a `ScriptPubKeyMan` and a `SolvingProvider`. This also has a bit of a performance impact on standard descriptor wallets, although less noticeable due to the small number of SPKMs.
As these functions are based on doing `IsMine` for each `Script
...
💬 ismaelsadeeq commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#discussion_r1194346683)
Concept ACK
Nit: As mentioned in review club, it would be nice to use a more descriptive variable name for the fr variable, considering your change is within the `transformNamedArguments` method. like find_result or a similar alternative.
```suggestion
auto find_result = argsIn.end();
```
(https://github.com/bitcoin/bitcoin/pull/26485#discussion_r1194346683)
Concept ACK
Nit: As mentioned in review club, it would be nice to use a more descriptive variable name for the fr variable, considering your change is within the `transformNamedArguments` method. like find_result or a similar alternative.
```suggestion
auto find_result = argsIn.end();
```
💬 mzumsande commented on pull request "p2p, rpc: Manual block-relay-only connections with addnode":
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1194346880)
Yes, thanks, that's a bit simpler and should be equivalent, so I took your suggestion!
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1194346880)
Yes, thanks, that's a bit simpler and should be equivalent, so I took your suggestion!
💬 mzumsande commented on pull request "p2p, rpc: Manual block-relay-only connections with addnode":
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1194347292)
made obsolete by the next comment.
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1194347292)
made obsolete by the next comment.
💬 mzumsande commented on pull request "p2p, rpc: Manual block-relay-only connections with addnode":
(https://github.com/bitcoin/bitcoin/pull/24170#issuecomment-1548552800)
[c13c986 ](https://github.com/bitcoin/bitcoin/commit/c13c9864c8f50a74dfca8feba42e2d28831decaf)to [41ce312](https://github.com/bitcoin/bitcoin/commit/41ce31289fdb521d3f3af1b67f750ce4a9196391):
Incorporated feedback by @brunoerg
(https://github.com/bitcoin/bitcoin/pull/24170#issuecomment-1548552800)
[c13c986 ](https://github.com/bitcoin/bitcoin/commit/c13c9864c8f50a74dfca8feba42e2d28831decaf)to [41ce312](https://github.com/bitcoin/bitcoin/commit/41ce31289fdb521d3f3af1b67f750ce4a9196391):
Incorporated feedback by @brunoerg
💬 brunoerg commented on pull request "walletdb: Remove unused CreateMockWalletDatabase":
(https://github.com/bitcoin/bitcoin/pull/27665#issuecomment-1548585079)
I've used `CreateMockWalletDatabase` in #27647, didn't know it hasn't been used in any other place, going to change it there.
(https://github.com/bitcoin/bitcoin/pull/27665#issuecomment-1548585079)
I've used `CreateMockWalletDatabase` in #27647, didn't know it hasn't been used in any other place, going to change it there.
👍 brunoerg approved a pull request: "walletdb: Remove unused CreateMockWalletDatabase"
(https://github.com/bitcoin/bitcoin/pull/27665#pullrequestreview-1427374841)
crACK 0282b2126dcc1216a25417db0716a3a28489b72d
(https://github.com/bitcoin/bitcoin/pull/27665#pullrequestreview-1427374841)
crACK 0282b2126dcc1216a25417db0716a3a28489b72d