🤔 Julianabrwon reviewed a pull request: "Use the stored CSubNet entry when unbanning"
(https://github.com/bitcoin-core/gui/pull/717#pullrequestreview-1407467727)
13VYk4xFjMN8d3wXnMZzvm8cwBHp3mo3LP
(https://github.com/bitcoin-core/gui/pull/717#pullrequestreview-1407467727)
13VYk4xFjMN8d3wXnMZzvm8cwBHp3mo3LP
📝 fanquake locked a pull request: "Use the stored CSubNet entry when unbanning"
(https://github.com/bitcoin-core/gui/pull/717)
The previous code visualized the `CSubNet` object as string, then parsed that string back to `CSubNet`. This is sub-optimal given that the original `CSubNet` object can be used directly instead.
This avoids calling `LookupSubNet()` from the GUI.
(https://github.com/bitcoin-core/gui/pull/717)
The previous code visualized the `CSubNet` object as string, then parsed that string back to `CSubNet`. This is sub-optimal given that the original `CSubNet` object can be used directly instead.
This avoids calling `LookupSubNet()` from the GUI.
⚠️ Sjors opened an issue: "Fuzz: compare our AES implementation to AES-NI "
(https://github.com/bitcoin/bitcoin/issues/27548)
We only use AES to generate a wallet encryption key from the user password. In #7689 we ditched the OpenSSL implementation for our own. It intentionally does not use special CPU instructions like AES-NI, because performance is not an issue for our use case. Instead it is based on an existing C implementation that's known to be constant-time.
We already have a fuzzer that checks an encryption - decryption round trip.
On CPU's that support it, we could add an additional fuzz target that uses
...
(https://github.com/bitcoin/bitcoin/issues/27548)
We only use AES to generate a wallet encryption key from the user password. In #7689 we ditched the OpenSSL implementation for our own. It intentionally does not use special CPU instructions like AES-NI, because performance is not an issue for our use case. Instead it is based on an existing C implementation that's known to be constant-time.
We already have a fuzzer that checks an encryption - decryption round trip.
On CPU's that support it, we could add an additional fuzz target that uses
...
💬 josibake commented on pull request "build: include example bitcoin.conf in build outputs under share/examples/ subdirectory":
(https://github.com/bitcoin/bitcoin/pull/25935#issuecomment-1529636777)
> Including the config file example in the root of the build output can suggest that editing it will have an effect on the running program.
I don't think this is true, and it is pretty clear in the documentation in the repo that you must move the config file to the datadir. For example, the file itself starts with:
```
##
## bitcoin.conf configuration file.
## Generated by contrib/devtools/gen-bitcoin-conf.sh.
##
## Lines beginning with # are comments.
## All possible configuration o
...
(https://github.com/bitcoin/bitcoin/pull/25935#issuecomment-1529636777)
> Including the config file example in the root of the build output can suggest that editing it will have an effect on the running program.
I don't think this is true, and it is pretty clear in the documentation in the repo that you must move the config file to the datadir. For example, the file itself starts with:
```
##
## bitcoin.conf configuration file.
## Generated by contrib/devtools/gen-bitcoin-conf.sh.
##
## Lines beginning with # are comments.
## All possible configuration o
...
💬 achow101 commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#issuecomment-1529640566)
ACK a5986e82dd2b8f923d60f9e31760ef62b9010105
(https://github.com/bitcoin/bitcoin/pull/27224#issuecomment-1529640566)
ACK a5986e82dd2b8f923d60f9e31760ef62b9010105
💬 achow101 commented on pull request "wallet: Replace `GetBalance()` logic with `AvailableCoins()`":
(https://github.com/bitcoin/bitcoin/pull/26756#issuecomment-1529645561)
I think this will end up making `GetBalance` really slow. We should really be tracking our UTXOs and computing the balance (and available coins) from that instead of iterating the entire wallet to figure out the UTXOs. #27286 implements the first steps to doing that, and I think we should prefer moving in that direction.
(https://github.com/bitcoin/bitcoin/pull/26756#issuecomment-1529645561)
I think this will end up making `GetBalance` really slow. We should really be tracking our UTXOs and computing the balance (and available coins) from that instead of iterating the entire wallet to figure out the UTXOs. #27286 implements the first steps to doing that, and I think we should prefer moving in that direction.
🚀 achow101 merged a pull request: "refactor: Remove CAddressBookData::destdata"
(https://github.com/bitcoin/bitcoin/pull/27224)
(https://github.com/bitcoin/bitcoin/pull/27224)
💬 achow101 commented on pull request "rpc, docs: Add note for commands that supports only legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/25680#issuecomment-1529648956)
ACK 9141e4395a5f923059ad61ac6ba42a1a89e92cb0
(https://github.com/bitcoin/bitcoin/pull/25680#issuecomment-1529648956)
ACK 9141e4395a5f923059ad61ac6ba42a1a89e92cb0
🚀 achow101 merged a pull request: "rpc, docs: Add note for commands that supports only legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/25680)
(https://github.com/bitcoin/bitcoin/pull/25680)
💬 furszy commented on pull request "wallet: finish addressbook encapsulation":
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1529669583)
rebased post #27224 merge, conflicts solved.
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1529669583)
rebased post #27224 merge, conflicts solved.
💬 achow101 commented on pull request "bumpfee: allow send coins back to yourself":
(https://github.com/bitcoin/bitcoin/pull/27195#issuecomment-1529669702)
ACK be72663a1521bc6cdf16d43a4feae7c5b57735c0
(https://github.com/bitcoin/bitcoin/pull/27195#issuecomment-1529669702)
ACK be72663a1521bc6cdf16d43a4feae7c5b57735c0
💬 josibake commented on pull request "wallet: improve rescan performance 640%":
(https://github.com/bitcoin/bitcoin/pull/26951#issuecomment-1529672330)
Concept -0
While this may be very useful for certain wallet usage patterns, this doesn't seem generally useful for _all_ wallets, which makes it hard to justify the review/complexity/maintenance burden.
> @achow101 These filters can be used for everything we use the other block filters for, I've only implemented the wallet query because that's the biggest win I see, but there's no reason they couldn't be used everywhere else (where there might be similar speed ups available).
Wallet r
...
(https://github.com/bitcoin/bitcoin/pull/26951#issuecomment-1529672330)
Concept -0
While this may be very useful for certain wallet usage patterns, this doesn't seem generally useful for _all_ wallets, which makes it hard to justify the review/complexity/maintenance burden.
> @achow101 These filters can be used for everything we use the other block filters for, I've only implemented the wallet query because that's the biggest win I see, but there's no reason they couldn't be used everywhere else (where there might be similar speed ups available).
Wallet r
...
🚀 achow101 merged a pull request: "bumpfee: allow send coins back to yourself"
(https://github.com/bitcoin/bitcoin/pull/27195)
(https://github.com/bitcoin/bitcoin/pull/27195)
💬 furszy commented on pull request "wallet: improve rescan performance 640%":
(https://github.com/bitcoin/bitcoin/pull/26951#issuecomment-1529674779)
> Rather than make a new index, if there are ways to improve the existing BlockFilter index, I think that would be much better as we would be improving two existing use cases without needing to maintain a new index.
@josibake, orthogonal topic but check #26966 and #27006 :).
(https://github.com/bitcoin/bitcoin/pull/26951#issuecomment-1529674779)
> Rather than make a new index, if there are ways to improve the existing BlockFilter index, I think that would be much better as we would be improving two existing use cases without needing to maintain a new index.
@josibake, orthogonal topic but check #26966 and #27006 :).
💬 0xB10C commented on pull request "Remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1529678174)
> Its original intention was to be publicly callable, but it is now (in Bitcoin Core) gated behind stricter Net Permissions which make it accessible to trusted peers only.
>
> Subsequently the service was gated behind the NetPermissionFlags::Mempool flag, meaning that the original BIP identitifcation method is no longer sufficient to determine that a node will offer this service to you (the node operator must whitelist or otherwise elevate your net permissions before they will respond to it).
...
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1529678174)
> Its original intention was to be publicly callable, but it is now (in Bitcoin Core) gated behind stricter Net Permissions which make it accessible to trusted peers only.
>
> Subsequently the service was gated behind the NetPermissionFlags::Mempool flag, meaning that the original BIP identitifcation method is no longer sufficient to determine that a node will offer this service to you (the node operator must whitelist or otherwise elevate your net permissions before they will respond to it).
...
💬 aureleoules commented on pull request "ci: Add test coverage job":
(https://github.com/bitcoin/bitcoin/pull/27547#issuecomment-1529678391)
A backwards compatibility test seems to fail because of BDB, I will fix it if this PR gets Concept ACKed.
(https://github.com/bitcoin/bitcoin/pull/27547#issuecomment-1529678391)
A backwards compatibility test seems to fail because of BDB, I will fix it if this PR gets Concept ACKed.
💬 achow101 commented on pull request "rpc: simplify scan blocks":
(https://github.com/bitcoin/bitcoin/pull/26780#issuecomment-1529689713)
ACK b922f6b5262884f42d7483f1e9af35650bdb53a7
(https://github.com/bitcoin/bitcoin/pull/26780#issuecomment-1529689713)
ACK b922f6b5262884f42d7483f1e9af35650bdb53a7
💬 josibake commented on pull request "Add fee_est tool for debugging fee estimation code":
(https://github.com/bitcoin/bitcoin/pull/10443#issuecomment-1529690116)
Haven't dug into the code, but I'm leaning toward Concept ACK, Approach NACK
As someone who is planning to do fee estimation research in the near future, I don't think I'd use something like this the way it's currently written. I would much prefer something that logs Bitcoin Core's live _fee rate_ estimations, or some option that would allow me to tell Bitcoin Core it's at a certain chain height, feed it some mempool data, and then ask it for fee estimations.
I also agree that the tool for
...
(https://github.com/bitcoin/bitcoin/pull/10443#issuecomment-1529690116)
Haven't dug into the code, but I'm leaning toward Concept ACK, Approach NACK
As someone who is planning to do fee estimation research in the near future, I don't think I'd use something like this the way it's currently written. I would much prefer something that logs Bitcoin Core's live _fee rate_ estimations, or some option that would allow me to tell Bitcoin Core it's at a certain chain height, feed it some mempool data, and then ask it for fee estimations.
I also agree that the tool for
...
🚀 achow101 merged a pull request: "rpc: simplify scan blocks"
(https://github.com/bitcoin/bitcoin/pull/26780)
(https://github.com/bitcoin/bitcoin/pull/26780)
💬 josibake commented on pull request "wallet: Replace `GetBalance()` logic with `AvailableCoins()`":
(https://github.com/bitcoin/bitcoin/pull/26756#issuecomment-1529697719)
I'd also prefer we stopped using `AvailableCoins` as a general wallet traversal tool in favor of something like https://github.com/bitcoin/bitcoin/pull/27286
A lot of bugs/strangeness in the wallet seems to come from the fact that we always need to iterate over a bunch of transactions to learn the state of our wallet. If we did track wallet state in one place, we could have well-scoped individual functions for specific tasks.
(https://github.com/bitcoin/bitcoin/pull/26756#issuecomment-1529697719)
I'd also prefer we stopped using `AvailableCoins` as a general wallet traversal tool in favor of something like https://github.com/bitcoin/bitcoin/pull/27286
A lot of bugs/strangeness in the wallet seems to come from the fact that we always need to iterate over a bunch of transactions to learn the state of our wallet. If we did track wallet state in one place, we could have well-scoped individual functions for specific tasks.