💬 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.
💬 achow101 commented on pull request "test: Add test for `sendmany` rpc that uses `subtractfeefrom` parameter":
(https://github.com/bitcoin/bitcoin/pull/26733#issuecomment-1529706820)
ACK 057057a2d7e23c2e29cbfd29a5124b3947a33757
(https://github.com/bitcoin/bitcoin/pull/26733#issuecomment-1529706820)
ACK 057057a2d7e23c2e29cbfd29a5124b3947a33757
🚀 fanquake merged a pull request: "test: add coverage for `-bantime`"
(https://github.com/bitcoin/bitcoin/pull/26604)
(https://github.com/bitcoin/bitcoin/pull/26604)
💬 michaelfolkson commented on pull request "Remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1529711107)
@0xB10C: Thanks for this, great analysis.
> I'm generally in favor of slimming down and removing the message (and also the BIP-37 implementation) at some point.
I read that as a longer term Concept ACK for removal but you'd like a deprecation release schedule rather than removing it immediately in say the next major version release. Presumably something like a deprecation warning in the next major version release before actual deprecation in the following major version release could work f
...
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1529711107)
@0xB10C: Thanks for this, great analysis.
> I'm generally in favor of slimming down and removing the message (and also the BIP-37 implementation) at some point.
I read that as a longer term Concept ACK for removal but you'd like a deprecation release schedule rather than removing it immediately in say the next major version release. Presumably something like a deprecation warning in the next major version release before actual deprecation in the following major version release could work f
...
🚀 achow101 merged a pull request: "test: Add test for `sendmany` rpc that uses `subtractfeefrom` parameter"
(https://github.com/bitcoin/bitcoin/pull/26733)
(https://github.com/bitcoin/bitcoin/pull/26733)