💬 ryanofsky commented on pull request "wallet: fully migrate address book entries for watchonly/solvable wallets":
(https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134615114)
In commit "wallet: fully migrate address book entries for watchonly/solvable wallets" (d5f4ae7fac0bceb0c9ad939b9a4fbdb85da0bf95)
It seems like a bug to not write the label when then label is empty, because will make receiving address with empty labels labels appear to be change addresses. If the label isn't written `CAddressBookData::SetLabel` will not be called:
https://github.com/bitcoin/bitcoin/blob/f50fb178c30ea4bec0b369af3d15cab08d33396f/src/wallet/wallet.h#L219-L222
so `CAddressBo
...
(https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134615114)
In commit "wallet: fully migrate address book entries for watchonly/solvable wallets" (d5f4ae7fac0bceb0c9ad939b9a4fbdb85da0bf95)
It seems like a bug to not write the label when then label is empty, because will make receiving address with empty labels labels appear to be change addresses. If the label isn't written `CAddressBookData::SetLabel` will not be called:
https://github.com/bitcoin/bitcoin/blob/f50fb178c30ea4bec0b369af3d15cab08d33396f/src/wallet/wallet.h#L219-L222
so `CAddressBo
...
💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1466964818)
> Could also add a test framework option to whitelist peers. So we don't have to add -whitelist flag everywhere.
Sounds good! Gonna implement it!
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1466964818)
> Could also add a test framework option to whitelist peers. So we don't have to add -whitelist flag everywhere.
Sounds good! Gonna implement it!
💬 furszy commented on pull request "wallet: fully migrate address book entries for watchonly/solvable wallets":
(https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134628591)
Check #26836, bug should be fixed there. Have re-written the process there without the `persist_address_book` calls.
(https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134628591)
Check #26836, bug should be fixed there. Have re-written the process there without the `persist_address_book` calls.
💬 ryanofsky commented on pull request "wallet: Migrate legacy wallets to descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/19602#discussion_r1134633292)
In commit "Implement MigrateLegacyToDescriptor" (0bf7b38bff422e7413bcd3dc0abe2568dd918ddc)
It seems like this should say `IsMine` not `!IsMine`. Also I'm not sure why `addr_pair.second.purpose == "receive"` check is necessary. It seems like just checking IsMine should be enough, and it would be less robust to rely on purpose field being present.
(https://github.com/bitcoin/bitcoin/pull/19602#discussion_r1134633292)
In commit "Implement MigrateLegacyToDescriptor" (0bf7b38bff422e7413bcd3dc0abe2568dd918ddc)
It seems like this should say `IsMine` not `!IsMine`. Also I'm not sure why `addr_pair.second.purpose == "receive"` check is necessary. It seems like just checking IsMine should be enough, and it would be less robust to rely on purpose field being present.
💬 ryanofsky commented on pull request "wallet: fully migrate address book entries for watchonly/solvable wallets":
(https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134636537)
Thanks will check it out. Also would be good to have a test for the bug.
(https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134636537)
Thanks will check it out. Also would be good to have a test for the bug.
💬 furszy commented on pull request "wallet: Migrate legacy wallets to descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/19602#discussion_r1134644466)
It's because the migration process divides data between different wallets.
It first deletes and unloads the legacy spkm from the main wallet (check the beginning of the function), and setup new descriptors. Then moves the data to two possible wallets; a solvable and a watch-only wallet.
So the `!IsMine` means that the record in the addressbook is from the legacy spkm (not loaded anymore), so it needs migration to the new wallets.
It also took me a while to get it while was rewriting it fo
...
(https://github.com/bitcoin/bitcoin/pull/19602#discussion_r1134644466)
It's because the migration process divides data between different wallets.
It first deletes and unloads the legacy spkm from the main wallet (check the beginning of the function), and setup new descriptors. Then moves the data to two possible wallets; a solvable and a watch-only wallet.
So the `!IsMine` means that the record in the addressbook is from the legacy spkm (not loaded anymore), so it needs migration to the new wallets.
It also took me a while to get it while was rewriting it fo
...
💬 furszy commented on pull request "wallet: fully migrate address book entries for watchonly/solvable wallets":
(https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134645263)
yeah 👌🏼.
(https://github.com/bitcoin/bitcoin/pull/26761#discussion_r1134645263)
yeah 👌🏼.
💬 ryanofsky commented on pull request "wallet: Replace use of purpose strings with an enum":
(https://github.com/bitcoin/bitcoin/pull/27217#issuecomment-1467021441)
I made some changes to this in a branch that could be adopted https://github.com/ryanofsky/bitcoin/commits/review.27217.1-edit
- Tries to split commits and add a lot of comments so this is easier to review
- Make enum a public wallet type instead of putting it in interfaces/ so wallet/ code does not depend on interfaces/wallet code and `interfaces::Wallet::AddressPurpose` becomes just `wallet::AddressPurpose`
- Replaced `std::optional<AddressPurpose>` with plain `AddressPurpose` in many pla
...
(https://github.com/bitcoin/bitcoin/pull/27217#issuecomment-1467021441)
I made some changes to this in a branch that could be adopted https://github.com/ryanofsky/bitcoin/commits/review.27217.1-edit
- Tries to split commits and add a lot of comments so this is easier to review
- Make enum a public wallet type instead of putting it in interfaces/ so wallet/ code does not depend on interfaces/wallet code and `interfaces::Wallet::AddressPurpose` becomes just `wallet::AddressPurpose`
- Replaced `std::optional<AddressPurpose>` with plain `AddressPurpose` in many pla
...
💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1134684312)
to add a second address, the test could do something like...
```
while (len(node.getnodeaddresses(count=0) < 2) {
node.addpeeraddress(...)
}
```
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1134684312)
to add a second address, the test could do something like...
```
while (len(node.getnodeaddresses(count=0) < 2) {
node.addpeeraddress(...)
}
```
📝 theStack opened a pull request: "qt: remove confusing "Dust" label from coincontrol dialog"
(https://github.com/bitcoin-core/gui/pull/719)
In contrast to to all other labels on the coin selection dialog, the displayed dust information has nothing to do with the selected coins. All that this label shows is whether at least one of the _outputs_ qualify as dust, but the outputs are set in a different dialog. (Even worse, the dust check is currently simply wrong because it only looks at an output's nValue and just assumes a P2PKH script size.)
As the label clearly doesn't help the user and is, quite the contrary, rather increasing c
...
(https://github.com/bitcoin-core/gui/pull/719)
In contrast to to all other labels on the coin selection dialog, the displayed dust information has nothing to do with the selected coins. All that this label shows is whether at least one of the _outputs_ qualify as dust, but the outputs are set in a different dialog. (Even worse, the dust check is currently simply wrong because it only looks at an output's nValue and just assumes a P2PKH script size.)
As the label clearly doesn't help the user and is, quite the contrary, rather increasing c
...
💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1134703332)
adding these addresses would be subject to the same sporadic failures that we've been discussing elsewhere. although `GetGroup` would return different values, they aren't isolated so will still occasionally hash to the same positions.
you have to add a while loop or something to keep trying until success to prevent sporadic failures
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1134703332)
adding these addresses would be subject to the same sporadic failures that we've been discussing elsewhere. although `GetGroup` would return different values, they aren't isolated so will still occasionally hash to the same positions.
you have to add a while loop or something to keep trying until success to prevent sporadic failures
💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1134705887)
hm, seems a little low-level for release notes. maybe something like
```suggestion
- CLI -addrinfo previously only returned a subset of addresses known to the node. It has been reworked to use the new `getaddrmaninfo` RPC and returns information about the entirety of the node's addrman. (#26988)
```
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1134705887)
hm, seems a little low-level for release notes. maybe something like
```suggestion
- CLI -addrinfo previously only returned a subset of addresses known to the node. It has been reworked to use the new `getaddrmaninfo` RPC and returns information about the entirety of the node's addrman. (#26988)
```
📝 pablomartin4btc opened a pull request: "httpserver, rest: fix segmentation fault on evhttp_uri_get_query"
(https://github.com/bitcoin/bitcoin/pull/27253)
The issue is that an invalid uri parsed by `evhttp_uri_parse()`(part of the [libevent open-source project]( https://github.com/libevent/libevent.)) which result (NULL/ nullptr) is sent to `evhttp_uri_get_query()` will produce a segmentation fault.
https://github.com/bitcoin-core/gui/blob/86bacd75e76eff207ef8f7bdb897365f5b6e7b6b/src/httpserver.cpp#L673-L676
In order to reproduce the issue:
1. Start `bitcoind -rest`.
2. Get a block hash (e.g.: `./src/bitcoin-cli getbestblockhash`).
3. Exe
...
(https://github.com/bitcoin/bitcoin/pull/27253)
The issue is that an invalid uri parsed by `evhttp_uri_parse()`(part of the [libevent open-source project]( https://github.com/libevent/libevent.)) which result (NULL/ nullptr) is sent to `evhttp_uri_get_query()` will produce a segmentation fault.
https://github.com/bitcoin-core/gui/blob/86bacd75e76eff207ef8f7bdb897365f5b6e7b6b/src/httpserver.cpp#L673-L676
In order to reproduce the issue:
1. Start `bitcoind -rest`.
2. Get a block hash (e.g.: `./src/bitcoin-cli getbestblockhash`).
3. Exe
...
🚀 fanquake merged a pull request: "refactor: Split logging utilities from system.h"
(https://github.com/bitcoin/bitcoin/pull/27238)
(https://github.com/bitcoin/bitcoin/pull/27238)
💬 MarcoFalke commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1135206140)
Are you sure? I tested my proposed change locally.
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1135206140)
Are you sure? I tested my proposed change locally.
💬 MarcoFalke commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1135213207)
Looks like it throws `test_framework.authproxy.JSONRPCException: Error: the wallet is currently being used to rescan the blockchain for related transactions. Please call `abortrescan` before locking the wallet. (-4)`, which can be caught by `assert_raises_rpc_error`?
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1135213207)
Looks like it throws `test_framework.authproxy.JSONRPCException: Error: the wallet is currently being used to rescan the blockchain for related transactions. Please call `abortrescan` before locking the wallet. (-4)`, which can be caught by `assert_raises_rpc_error`?
💬 glozow commented on pull request "mempool: Add mempool tracepoints":
(https://github.com/bitcoin/bitcoin/pull/26531#discussion_r1119840003)
I have a question about reusing an event name, i.e. for rejections in packages (in a future PR) could/should we add a `TRACE2(mempool, rejected, txhash, reason)` or `TRACE3(mempool, rejected, txhash, pkghash, reason)` in the package rejection code? Or should we use a different `event`?
(https://github.com/bitcoin/bitcoin/pull/26531#discussion_r1119840003)
I have a question about reusing an event name, i.e. for rejections in packages (in a future PR) could/should we add a `TRACE2(mempool, rejected, txhash, reason)` or `TRACE3(mempool, rejected, txhash, pkghash, reason)` in the package rejection code? Or should we use a different `event`?
🚀 hebasto merged a pull request: "Mask values on Transactions View"
(https://github.com/bitcoin-core/gui/pull/708)
(https://github.com/bitcoin-core/gui/pull/708)
📝 TheCharlatan opened a pull request: "refactor: Extract util/fs from util/system"
(https://github.com/bitcoin/bitcoin/pull/27254)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". This commit was originally authored by @empact and are taken from their parent PR #25152.
#### Context
There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel library
...
(https://github.com/bitcoin/bitcoin/pull/27254)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". This commit was originally authored by @empact and are taken from their parent PR #25152.
#### Context
There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel library
...
💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1467916011)
Rebased a7afa98ca38fd66fc69b77b95a6a57d50207911b -> 45048c0d7e0ee12210d226239fb5ce63d9ed4441 ([tc/2022-09-libbitcoinkernel-chainparams-args_5](https://github.com/TheCharlatan/bitcoin/commits/tc/2022-09-libbitcoinkernel-chainparams-args_5) -> [tc/2022-09-libbitcoinkernel-chainparams-args_6](https://github.com/TheCharlatan/bitcoin/commits/tc/2022-09-libbitcoinkernel-chainparams-args_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/tc/2022-09-libbitcoinkernel-chainparams-args_5..tc/202
...
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1467916011)
Rebased a7afa98ca38fd66fc69b77b95a6a57d50207911b -> 45048c0d7e0ee12210d226239fb5ce63d9ed4441 ([tc/2022-09-libbitcoinkernel-chainparams-args_5](https://github.com/TheCharlatan/bitcoin/commits/tc/2022-09-libbitcoinkernel-chainparams-args_5) -> [tc/2022-09-libbitcoinkernel-chainparams-args_6](https://github.com/TheCharlatan/bitcoin/commits/tc/2022-09-libbitcoinkernel-chainparams-args_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/tc/2022-09-libbitcoinkernel-chainparams-args_5..tc/202
...