💬 hernanmarino commented on issue "Confusing/misleading "Dust:" label in coin selection dialog":
(https://github.com/bitcoin-core/gui/issues/699#issuecomment-1466831109)
+1 on removing it
(https://github.com/bitcoin-core/gui/issues/699#issuecomment-1466831109)
+1 on removing it
💬 mzumsande commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1466870416)
> Hmm, this is maybe ok. Another approach would be to try to open to e.g. Tor while still having < 8 outbound connections. To counter the issue of staying with < 8 connections and not being able to open to Tor because e.g. the Tor proxy is broken, then we could give up after a few attempts and then fall back to the old way of filling with random (aka IPv4 ;-)) addresses. If we overshoot the 8 connections (like described above), then for some time we will have more, until eviction kicks in. How m
...
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1466870416)
> Hmm, this is maybe ok. Another approach would be to try to open to e.g. Tor while still having < 8 outbound connections. To counter the issue of staying with < 8 connections and not being able to open to Tor because e.g. the Tor proxy is broken, then we could give up after a few attempts and then fall back to the old way of filling with random (aka IPv4 ;-)) addresses. If we overshoot the 8 connections (like described above), then for some time we will have more, until eviction kicks in. How m
...
💬 mzumsande commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1466872077)
> > Why it doesn't make sense? I think it is an extra effort to obtain IPv4 addresses if one has IPv6 addresses and the other way around. Why impossible in some network environments? After all you know if you are going to attempt to connect to 5.6.7.8 or 2030:c0fe:....
>
> I also not sure if I agree with counting IPv4 and IPv6 as one network and I think this could need a bit more reasoning. Is it because they are both clearnet? Is it a workaround to don't have 8 protected peers if all network
...
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1466872077)
> > Why it doesn't make sense? I think it is an extra effort to obtain IPv4 addresses if one has IPv6 addresses and the other way around. Why impossible in some network environments? After all you know if you are going to attempt to connect to 5.6.7.8 or 2030:c0fe:....
>
> I also not sure if I agree with counting IPv4 and IPv6 as one network and I think this could need a bit more reasoning. Is it because they are both clearnet? Is it a workaround to don't have 8 protected peers if all network
...
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1134606456)
Thanks, that’s unexpected. I would also expect that `CalculateTotalBumpFees(…)` always is equal or smaller to the sum of individual bump fees. I’ll check it out.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1134606456)
Thanks, that’s unexpected. I would also expect that `CalculateTotalBumpFees(…)` always is equal or smaller to the sum of individual bump fees. I’ll check it out.
💬 willcl-ark commented on issue "untrust vs unsafe ?":
(https://github.com/bitcoin/bitcoin/issues/18625#issuecomment-1466951596)
I think a breaking change to the RPC argument keys, seeing as we accept named arguments via rpc, would likely not be considered worth it just to harmonise vocabulary.
@achow101 I'd be curious of your thoughts here? If we aren't going to change this then perhaps we can close the issue.
(https://github.com/bitcoin/bitcoin/issues/18625#issuecomment-1466951596)
I think a breaking change to the RPC argument keys, seeing as we accept named arguments via rpc, would likely not be considered worth it just to harmonise vocabulary.
@achow101 I'd be curious of your thoughts here? If we aren't going to change this then perhaps we can close the issue.
💬 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.