💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032031)
Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032031)
Done as suggested.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032083)
Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032083)
Done as suggested.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032129)
Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032129)
Done as suggested.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032152)
Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032152)
Done as suggested.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032186)
Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1211032186)
Done as suggested.
📝 achow101 opened a pull request: "walletdb: Add PrefixCursor"
(https://github.com/bitcoin/bitcoin/pull/27790)
Split from #24914 as suggested in https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1442091917
This PR adds a database cursor that gives a view over all of the records beginning with the same prefix.
(https://github.com/bitcoin/bitcoin/pull/27790)
Split from #24914 as suggested in https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1442091917
This PR adds a database cursor that gives a view over all of the records beginning with the same prefix.
💬 achow101 commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1569422995)
> 1. The main thing would be making the "walletdb: refactor X loading" commits self contained and not leave behind dead code that needs to be cleaned up later. It's hard to identify what code is being replaced in current commits when not all the code being replaced is actually removed.
I've implemented your suggestions and some additional dead code cleanups.
> 2. I think it might be good to move first 2 commits up to "wallet: Add GetPrefixCursor to DatabaseBatch" ([422a843](https://git
...
(https://github.com/bitcoin/bitcoin/pull/24914#issuecomment-1569422995)
> 1. The main thing would be making the "walletdb: refactor X loading" commits self contained and not leave behind dead code that needs to be cleaned up later. It's hard to identify what code is being replaced in current commits when not all the code being replaced is actually removed.
I've implemented your suggestions and some additional dead code cleanups.
> 2. I think it might be good to move first 2 commits up to "wallet: Add GetPrefixCursor to DatabaseBatch" ([422a843](https://git
...
📝 achow101 converted_to_draft a pull request: "wallet: Load database records in a particular order"
(https://github.com/bitcoin/bitcoin/pull/24914)
Currently when we load a wallet, we just iterate through all of the records in the database and add them completely statelessly. However we have some records which do rely on other records being loaded before they are. To deal with this, we use `CWalletScanState` to hold things temporarily until all of the records have been read and then we load the stateful things.
However this can be slow, and with some future improvements, can cause some pretty drastic slowdowns to retain this pattern. So
...
(https://github.com/bitcoin/bitcoin/pull/24914)
Currently when we load a wallet, we just iterate through all of the records in the database and add them completely statelessly. However we have some records which do rely on other records being loaded before they are. To deal with this, we use `CWalletScanState` to hold things temporarily until all of the records have been read and then we load the stateful things.
However this can be slow, and with some future improvements, can cause some pretty drastic slowdowns to retain this pattern. So
...
💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569426939)
> (I wouldn't expect bitcoin-cli to convert the types)
Then this ends up being unusable, e.g.:
```
$ src/bitcoin-cli -regtest -rpcwallet=funds -named send '[{"bcrt1q4ul7y0p3p9d6sx6atnq9kucxng7jpde9tqfrt6":1}]' change_position=0
error code: -3
error message:
JSON value of type null is not of expected type string
```
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569426939)
> (I wouldn't expect bitcoin-cli to convert the types)
Then this ends up being unusable, e.g.:
```
$ src/bitcoin-cli -regtest -rpcwallet=funds -named send '[{"bcrt1q4ul7y0p3p9d6sx6atnq9kucxng7jpde9tqfrt6":1}]' change_position=0
error code: -3
error message:
JSON value of type null is not of expected type string
```
💬 ajtowns commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569448946)
> > (I wouldn't expect bitcoin-cli to convert the types)
>
> Then this ends up being fairly useless, e.g.:
>
> ```
> $ src/bitcoin-cli -regtest -rpcwallet=funds -named send '[{"bcrt1q4ul7y0p3p9d6sx6atnq9kucxng7jpde9tqfrt6":1}]' change_position=0
> error code: -3
> error message:
> JSON value of type null is not of expected type string
> ```
Do you mean `JSON value of type string for field change_position is not of expected type number` ? (If not, I can't duplicate your error...)
...
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569448946)
> > (I wouldn't expect bitcoin-cli to convert the types)
>
> Then this ends up being fairly useless, e.g.:
>
> ```
> $ src/bitcoin-cli -regtest -rpcwallet=funds -named send '[{"bcrt1q4ul7y0p3p9d6sx6atnq9kucxng7jpde9tqfrt6":1}]' change_position=0
> error code: -3
> error message:
> JSON value of type null is not of expected type string
> ```
Do you mean `JSON value of type string for field change_position is not of expected type number` ? (If not, I can't duplicate your error...)
...
💬 achow101 commented on pull request "RPC: Accept options as named-only parameters":
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569453818)
> Do you mean `JSON value of type string for field change_position is not of expected type number` ? (If not, I can't duplicate your error...)
Oops, yes, I don't think I was on the HEAD commit.
> Yeah, that does suck, and I don't see an obvious fix -- presumably we'd want named-only parameters to be treated as json rather than a string (unless overridden), but if it's not overridden, then bitcoin-cli has no way of knowing if `change_position` here is a positional param or a named one...
...
(https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1569453818)
> Do you mean `JSON value of type string for field change_position is not of expected type number` ? (If not, I can't duplicate your error...)
Oops, yes, I don't think I was on the HEAD commit.
> Yeah, that does suck, and I don't see an obvious fix -- presumably we'd want named-only parameters to be treated as json rather than a string (unless overridden), but if it's not overridden, then bitcoin-cli has no way of knowing if `change_position` here is a positional param or a named one...
...
💬 joeyvee1986 commented on pull request "walletdb: Add PrefixCursor":
(https://github.com/bitcoin/bitcoin/pull/27790#issuecomment-1569477575)
Andrew or anyone, where can I find your IRC/Discord/Telegram Chan?
On Tue, May 30, 2023, 10:00 PM Andrew Chow ***@***.***> wrote:
> Split from #24914 <https://github.com/bitcoin/bitcoin/pull/24914> as
> suggested in #24914 (review)
> <https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1442091917>
>
> This PR adds a database cursor that gives a view over all of the records
> beginning with the same prefix.
> ------------------------------
> You can view, comment on, or merge this p
...
(https://github.com/bitcoin/bitcoin/pull/27790#issuecomment-1569477575)
Andrew or anyone, where can I find your IRC/Discord/Telegram Chan?
On Tue, May 30, 2023, 10:00 PM Andrew Chow ***@***.***> wrote:
> Split from #24914 <https://github.com/bitcoin/bitcoin/pull/24914> as
> suggested in #24914 (review)
> <https://github.com/bitcoin/bitcoin/pull/24914#pullrequestreview-1442091917>
>
> This PR adds a database cursor that gives a view over all of the records
> beginning with the same prefix.
> ------------------------------
> You can view, comment on, or merge this p
...
💬 MarcoFalke commented on pull request "fuzz: fix wallet notifications.cpp":
(https://github.com/bitcoin/bitcoin/pull/27786#issuecomment-1569541366)
lgtm ACK a10f032115644d60a4321af06768d87d625affc1, thanks
(https://github.com/bitcoin/bitcoin/pull/27786#issuecomment-1569541366)
lgtm ACK a10f032115644d60a4321af06768d87d625affc1, thanks
💬 MarcoFalke commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1569542166)
> The base fuzzing corpus should have detected it right?.
Yes, but the fuzz target is too slow right now, so the fuzz input directory is empty.
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1569542166)
> The base fuzzing corpus should have detected it right?.
Yes, but the fuzz target is too slow right now, so the fuzz input directory is empty.
💬 MarcoFalke commented on pull request "test: fix intermittent error in getblockfrompeer.py":
(https://github.com/bitcoin/bitcoin/pull/27784#issuecomment-1569544891)
lgtm ACK 9fe9074266c6d7ddb40384d81741df1fc94980ff
(https://github.com/bitcoin/bitcoin/pull/27784#issuecomment-1569544891)
lgtm ACK 9fe9074266c6d7ddb40384d81741df1fc94980ff
💬 MarcoFalke commented on pull request "Add public Boost headers explicitly":
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1211170115)
Feel free to take:
```diff
diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
index a5394d8816..b1d67c1432 100644
--- a/src/wallet/test/coinselector_tests.cpp
+++ b/src/wallet/test/coinselector_tests.cpp
@@ -432,7 +432,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
CAmount selection_target = 16 * CENT;
const auto& no_res = SelectCoinsBnB(GroupCoins(available_coins.All(), /*subtract_fee_outputs*/true),
...
(https://github.com/bitcoin/bitcoin/pull/27783#discussion_r1211170115)
Feel free to take:
```diff
diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
index a5394d8816..b1d67c1432 100644
--- a/src/wallet/test/coinselector_tests.cpp
+++ b/src/wallet/test/coinselector_tests.cpp
@@ -432,7 +432,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
CAmount selection_target = 16 * CENT;
const auto& no_res = SelectCoinsBnB(GroupCoins(available_coins.All(), /*subtract_fee_outputs*/true),
...
💬 Ayush170-Future commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1211172119)
`random_mutable_transaction` appears to be of no use.
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1211172119)
`random_mutable_transaction` appears to be of no use.
📝 denavila opened a pull request: "wallet: Deniability API (Unilateral Transaction Meta-Privacy)"
(https://github.com/bitcoin/bitcoin/pull/27792)
This PR is the wallet API and implementation portion of the GUI PR ( https://github.com/bitcoin-core/gui/pull/733 ) which is an implementation of the ideas in Paul Sztorc's blog post "**Deniability - Unilateral Transaction Meta-Privacy**"(https://www.truthcoin.info/blog/deniability/).
The GUI PR has all the details and screenshots of the GUI additions. Here I'll just copy the relevant context for the wallet API changes:
"_In short, Paul's idea is to periodically split coins and send them t
...
(https://github.com/bitcoin/bitcoin/pull/27792)
This PR is the wallet API and implementation portion of the GUI PR ( https://github.com/bitcoin-core/gui/pull/733 ) which is an implementation of the ideas in Paul Sztorc's blog post "**Deniability - Unilateral Transaction Meta-Privacy**"(https://www.truthcoin.info/blog/deniability/).
The GUI PR has all the details and screenshots of the GUI additions. Here I'll just copy the relevant context for the wallet API changes:
"_In short, Paul's idea is to periodically split coins and send them t
...
💬 denavila commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1569687477)
@achow101 - per our discussion, I've created a PR with the core "deniability" functionality in the main repo here. The GUI portion is rebased over this branch and is in the GUI repo. I've updated the PRs' descriptions to point to each other.
Please let me know what you think. Thanks!
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1569687477)
@achow101 - per our discussion, I've created a PR with the core "deniability" functionality in the main repo here. The GUI portion is rebased over this branch and is in the GUI repo. I've updated the PRs' descriptions to point to each other.
Please let me know what you think. Thanks!
💬 MarcoFalke commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1569702764)
Not sure how much this achieves. Bitcoin Core's wallet has a "unique" fingerprint on transactions that is shared by way less than 50% of transactions in the network, so creating additional transactions where it is likely (latest when the outputs are spent) that it was a self-transfer seems only to be bloating the utxo set and chain space? I haven't thought about this, but it would be good to have a more in-depth analysis of this, considering using a different "fingerprint vector".
(https://github.com/bitcoin/bitcoin/pull/27792#issuecomment-1569702764)
Not sure how much this achieves. Bitcoin Core's wallet has a "unique" fingerprint on transactions that is shared by way less than 50% of transactions in the network, so creating additional transactions where it is likely (latest when the outputs are spent) that it was a self-transfer seems only to be bloating the utxo set and chain space? I haven't thought about this, but it would be good to have a more in-depth analysis of this, considering using a different "fingerprint vector".