Wallet: Add foreign_outputs metadata to support CoinJoin transactions #25991

pull luke-jr wants to merge 10 commits into bitcoin:master from luke-jr:wallet_foreign_outputs_metadata changing 14 files +400 −19
  1. luke-jr commented at 0:49 am on September 3, 2022: member

    Fixes #14136

    Does not fix GUI (can be done in GUI-side PR afterward)

  2. QA: wallet_listtransactions: Test for current behaviour for coinjoins ad3300d868
  3. luke-jr force-pushed on Sep 3, 2022
  4. luke-jr force-pushed on Sep 3, 2022
  5. DrahtBot added the label Wallet on Sep 3, 2022
  6. ghost commented at 8:09 am on September 3, 2022: none

    Concept ACK.

    Will test today.

    Edit: It’s not entirely clear at this point how to avoid older versions from opening a wallet with this metadata. Advice?

    Not sure about this. If there is any error in old wallets that should work. Users with such issues can post on stackexchange, reddit etc.

    Can also add information in release notes.

  7. DrahtBot commented at 2:44 pm on September 3, 2022: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK ghost

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)
    • #27224 (refactor: Remove CAddressBookData::destdata by ryanofsky)
    • #25979 ([WIP] wallet: standardize change output detection process by furszy)
    • #25297 (wallet: group independent db writes on single batched db transaction by furszy)
    • #24914 (wallet: Load database records in a particular order by achow101)
    • #18919 (test: Add gettransaction test for “coin-join” tx by MarcoFalke)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  8. ghost commented at 3:24 pm on September 3, 2022: none

    I don’t think this fixes #14136

    I tried it with my one of my signet wallet that was used for testing of joinstr transaction: 75e490b10b15a6a0422f25ff66ad98ef70390c8fecaac02712705dce8cc3564b

     0$ bitcoin-cli -rpcwallet="w5" gettransaction 75e490b10b15a6a0422f25ff66ad98ef70390c8fecaac02712705dce8cc3564b
 1{
 2  "amount": -0.00583000,
 3  "fee": 0.00465000,
 4  "confirmations": 2160,
 5  "blockhash": "00000007567d87fadb4673d0cb876cb1b7b9dbfedc1d9a0e40af7380d9da8435",
 6  "blockheight": 104271,
 7  "blockindex": 1,
 8  "blocktime": 1660978126,
 9  "txid": "75e490b10b15a6a0422f25ff66ad98ef70390c8fecaac02712705dce8cc3564b",
10  "wtxid": "e4b91ce21961a9e5498e2ff396f5e09fa8c7f3a36ef77abd4207f71855599990",
11  "walletconflicts": [
12  ],
13  "time": 1660976747,
14  "timereceived": 1660976747,
15  "bip125-replaceable": "no",
16  "details": [
17    {
18      "address": "tb1qhxrp4zl54ul0twtyz0gury5399q7z0kvqqrl6m",
19      "category": "send",
20      "amount": -0.00116600,
21      "vout": 0,
22      "fee": 0.00465000,
23      "abandoned": false
24    },
25    {
26      "address": "tb1qnnflzn8gwadhwxr46zpxgc8fy429t25w35wah9",
27      "category": "send",
28      "amount": -0.00116600,
29      "vout": 1,
30      "fee": 0.00465000,
31      "abandoned": false
32    },
33    {
34      "address": "tb1q85ny57lu8g7na23qt7tgf03srksh9x3hrpgkvq",
35      "category": "send",
36      "amount": -0.00116600,
37      "vout": 2,
38      "fee": 0.00465000,
39      "abandoned": false
40    },
41    {
42      "address": "tb1qe36279395n6crhx3d09cptvnv72chagd336jla",
43      "category": "send",
44      "amount": -0.00116600,
45      "vout": 3,
46      "fee": 0.00465000,
47      "abandoned": false
48    },
49    {
50      "address": "tb1qkdeccjk32n0muv45xp67kq2pfd3qvfe6wym0ey",
51      "category": "send",
52      "amount": -0.00116600,
53      "vout": 4,
54      "fee": 0.00465000,
55      "abandoned": false
56    }
57  ],
58  "hex": "02000000000105b4c6a8258717bce1b92f761acf76324bb808b78879ff3ccead1452de1ad5bf0a0000000000ffffffffa8f3781e6929b82fbe8a4e821497932266a2c051b20324baba0d6ae33ffddbf00100000000ffffffff3105dc25c8bd51e921aecda35b35ca0f8ab7239f5d3452c969ce75731fcdc0f00100000000ffffffffc0c6b85c082753edfd21e9fe286f6b62dbfc6cb30d61a92667263f4057f02d170000000000fffffffff9338d880fab8b9904dbdad9c51e737b50ca670c3db9b8df195fb03e18d48cdd0100000000ffffffff0578c7010000000000160014b9861a8bf4af3ef5b96413d1c192912941e13ecc78c70100000000001600149cd3f14ce8775b771875d0826460e9255455aa8e78c70100000000001600143d264a7bfc3a3d3eaa205f9684be301da1729a3778c7010000000000160014cc74af1625a4f581dcd16bcb80ad9367958bf50d78c7010000000000160014b3738c4ad154dfbe32b43075eb01414b6206273a02473044022038884ba02e77e3c53c62412be065352b8c96b2420e117816e16b24fd6d9c47b202204257aad3ae0c9e42930ad2bb024b5adcda6f876c95b3772ae2ffa72a28327382012102449be5fb74725255eeeb50eba930fa87705f21e99d13cd710cf2c1f21153c8080247304402200a29d109643ddfb04fd26503c7e9887c9eabdc5f0ecc5072a357b91cd669ee8b022060c211a7c33eebcee308c641988e8cc67dbbe3b65856d4c69a2501a8694b0187012102131ef894942f0f9fe8f3455084c17f010549a7702bbfd74e4dff489738d0827f024730440220433cc9d973edfc3def7ec6afc49e18551e6a8af3267b75b6b72081bb80f99bbe022022473db462bcb6b698928c27fbda8bfe616cab07a62090e7dd69c4dd3a872523012103ac5d971ae3625f58c3ef5bcac46825653882bbb6a466f84d7226489dd876a1510247304402201f84bd62ab8e9986834da1f4e5e68c65d32cae7c6e2de69aa79666a53ea3696c022071a187d7248814080474e7c3a3fbbf771158f1e1c50e7d4e3c31a633f3af53fe012103025dc12c2024927ccbacb7f383d9a1a7d0709aaf73c7b22b794b82927f3be81a02473044022042636fb3f212af504b3196f27669c213342996f798aae6f50e4da0349f44c13e022076ce03586a0b6e387f330f6ef2ffc2fa952fca8c1391e0f68287ac9e17ff8381012103459928810bb656190ec36ca492f1e9c9ed84cc6ae09a0d2e9c62168bee2745b800000000"
59}
  9. luke-jr commented at 5:19 pm on September 3, 2022: member

    @1440000bytes Did you provide it with the necessary foreign output metadata? (It’s impossible to fix without that)

    eg (check the outputs specified are what you intend)

    0bitcoin-cli -rpcwallet="w5" submitrawtransactiontowallet 02000000000105b4c6a8258717bce1b92f761acf76324bb808b78879ff3ccead1452de1ad5bf0a0000000000ffffffffa8f3781e6929b82fbe8a4e821497932266a2c051b20324baba0d6ae33ffddbf00100000000ffffffff3105dc25c8bd51e921aecda35b35ca0f8ab7239f5d3452c969ce75731fcdc0f00100000000ffffffffc0c6b85c082753edfd21e9fe286f6b62dbfc6cb30d61a92667263f4057f02d170000000000fffffffff9338d880fab8b9904dbdad9c51e737b50ca670c3db9b8df195fb03e18d48cdd0100000000ffffffff0578c7010000000000160014b9861a8bf4af3ef5b96413d1c192912941e13ecc78c70100000000001600149cd3f14ce8775b771875d0826460e9255455aa8e78c70100000000001600143d264a7bfc3a3d3eaa205f9684be301da1729a3778c7010000000000160014cc74af1625a4f581dcd16bcb80ad9367958bf50d78c7010000000000160014b3738c4ad154dfbe32b43075eb01414b6206273a02473044022038884ba02e77e3c53c62412be065352b8c96b2420e117816e16b24fd6d9c47b202204257aad3ae0c9e42930ad2bb024b5adcda6f876c95b3772ae2ffa72a28327382012102449be5fb74725255eeeb50eba930fa87705f21e99d13cd710cf2c1f21153c8080247304402200a29d109643ddfb04fd26503c7e9887c9eabdc5f0ecc5072a357b91cd669ee8b022060c211a7c33eebcee308c641988e8cc67dbbe3b65856d4c69a2501a8694b0187012102131ef894942f0f9fe8f3455084c17f010549a7702bbfd74e4dff489738d0827f024730440220433cc9d973edfc3def7ec6afc49e18551e6a8af3267b75b6b72081bb80f99bbe022022473db462bcb6b698928c27fbda8bfe616cab07a62090e7dd69c4dd3a872523012103ac5d971ae3625f58c3ef5bcac46825653882bbb6a466f84d7226489dd876a1510247304402201f84bd62ab8e9986834da1f4e5e68c65d32cae7c6e2de69aa79666a53ea3696c022071a187d7248814080474e7c3a3fbbf771158f1e1c50e7d4e3c31a633f3af53fe012103025dc12c2024927ccbacb7f383d9a1a7d0709aaf73c7b22b794b82927f3be81a02473044022042636fb3f212af504b3196f27669c213342996f798aae6f50e4da0349f44c13e022076ce03586a0b6e387f330f6ef2ffc2fa952fca8c1391e0f68287ac9e17ff8381012103459928810bb656190ec36ca492f1e9c9ed84cc6ae09a0d2e9c62168bee2745b800000000 '{"foreign_outputs": [0,1,2,3]}'
  10. luke-jr force-pushed on Sep 3, 2022
  11. ghost commented at 6:13 pm on September 3, 2022: none

    @1440000bytes Did you provide it with the necessary foreign output metadata? (It’s impossible to fix without that)

    eg (check the outputs specified are what you intend)

    0bitcoin-cli -rpcwallet="w5" submitrawtransactiontowallet 02000000000105b4c6a8258717bce1b92f761acf76324bb808b78879ff3ccead1452de1ad5bf0a0000000000ffffffffa8f3781e6929b82fbe8a4e821497932266a2c051b20324baba0d6ae33ffddbf00100000000ffffffff3105dc25c8bd51e921aecda35b35ca0f8ab7239f5d3452c969ce75731fcdc0f00100000000ffffffffc0c6b85c082753edfd21e9fe286f6b62dbfc6cb30d61a92667263f4057f02d170000000000fffffffff9338d880fab8b9904dbdad9c51e737b50ca670c3db9b8df195fb03e18d48cdd0100000000ffffffff0578c7010000000000160014b9861a8bf4af3ef5b96413d1c192912941e13ecc78c70100000000001600149cd3f14ce8775b771875d0826460e9255455aa8e78c70100000000001600143d264a7bfc3a3d3eaa205f9684be301da1729a3778c7010000000000160014cc74af1625a4f581dcd16bcb80ad9367958bf50d78c7010000000000160014b3738c4ad154dfbe32b43075eb01414b6206273a02473044022038884ba02e77e3c53c62412be065352b8c96b2420e117816e16b24fd6d9c47b202204257aad3ae0c9e42930ad2bb024b5adcda6f876c95b3772ae2ffa72a28327382012102449be5fb74725255eeeb50eba930fa87705f21e99d13cd710cf2c1f21153c8080247304402200a29d109643ddfb04fd26503c7e9887c9eabdc5f0ecc5072a357b91cd669ee8b022060c211a7c33eebcee308c641988e8cc67dbbe3b65856d4c69a2501a8694b0187012102131ef894942f0f9fe8f3455084c17f010549a7702bbfd74e4dff489738d0827f024730440220433cc9d973edfc3def7ec6afc49e18551e6a8af3267b75b6b72081bb80f99bbe022022473db462bcb6b698928c27fbda8bfe616cab07a62090e7dd69c4dd3a872523012103ac5d971ae3625f58c3ef5bcac46825653882bbb6a466f84d7226489dd876a1510247304402201f84bd62ab8e9986834da1f4e5e68c65d32cae7c6e2de69aa79666a53ea3696c022071a187d7248814080474e7c3a3fbbf771158f1e1c50e7d4e3c31a633f3af53fe012103025dc12c2024927ccbacb7f383d9a1a7d0709aaf73c7b22b794b82927f3be81a02473044022042636fb3f212af504b3196f27669c213342996f798aae6f50e4da0349f44c13e022076ce03586a0b6e387f330f6ef2ffc2fa952fca8c1391e0f68287ac9e17ff8381012103459928810bb656190ec36ca492f1e9c9ed84cc6ae09a0d2e9c62168bee2745b800000000 '{"foreign_outputs": [0,1,2,3]}'

    So I need to run submitrawtransactiontowallet for every coinjoin?

  12. luke-jr commented at 6:29 pm on September 3, 2022: member
    Yes
  13. ghost commented at 6:40 pm on September 3, 2022: none

    Yes

    This is still wrong in my case because w5 wallet did not send amount displayed in the json:

    0$ bitcoin-cli -rpcwallet="w5" submitrawtransactiontowallet 02000000000105b4c6a8258717bce1b92f761acf76324bb808b78879ff3ccead1452de1ad5bf0a0000000000ffffffffa8f3781e6929b82fbe8a4e821497932266a2c051b20324baba0d6ae33ffddbf00100000000ffffffff3105dc25c8bd51e921aecda35b35ca0f8ab7239f5d3452c969ce75731fcdc0f00100000000ffffffffc0c6b85c082753edfd21e9fe286f6b62dbfc6cb30d61a92667263f4057f02d170000000000fffffffff9338d880fab8b9904dbdad9c51e737b50ca670c3db9b8df195fb03e18d48cdd0100000000ffffffff0578c7010000000000160014b9861a8bf4af3ef5b96413d1c192912941e13ecc78c70100000000001600149cd3f14ce8775b771875d0826460e9255455aa8e78c70100000000001600143d264a7bfc3a3d3eaa205f9684be301da1729a3778c7010000000000160014cc74af1625a4f581dcd16bcb80ad9367958bf50d78c7010000000000160014b3738c4ad154dfbe32b43075eb01414b6206273a02473044022038884ba02e77e3c53c62412be065352b8c96b2420e117816e16b24fd6d9c47b202204257aad3ae0c9e42930ad2bb024b5adcda6f876c95b3772ae2ffa72a28327382012102449be5fb74725255eeeb50eba930fa87705f21e99d13cd710cf2c1f21153c8080247304402200a29d109643ddfb04fd26503c7e9887c9eabdc5f0ecc5072a357b91cd669ee8b022060c211a7c33eebcee308c641988e8cc67dbbe3b65856d4c69a2501a8694b0187012102131ef894942f0f9fe8f3455084c17f010549a7702bbfd74e4dff489738d0827f024730440220433cc9d973edfc3def7ec6afc49e18551e6a8af3267b75b6b72081bb80f99bbe022022473db462bcb6b698928c27fbda8bfe616cab07a62090e7dd69c4dd3a872523012103ac5d971ae3625f58c3ef5bcac46825653882bbb6a466f84d7226489dd876a1510247304402201f84bd62ab8e9986834da1f4e5e68c65d32cae7c6e2de69aa79666a53ea3696c022071a187d7248814080474e7c3a3fbbf771158f1e1c50e7d4e3c31a633f3af53fe012103025dc12c2024927ccbacb7f383d9a1a7d0709aaf73c7b22b794b82927f3be81a02473044022042636fb3f212af504b3196f27669c213342996f798aae6f50e4da0349f44c13e022076ce03586a0b6e387f330f6ef2ffc2fa952fca8c1391e0f68287ac9e17ff8381012103459928810bb656190ec36ca492f1e9c9ed84cc6ae09a0d2e9c62168bee2745b800000000 '{"foreign_outputs": [0,1,2,3]}'

     0
 1$ bitcoin-cli -rpcwallet="w5" gettransaction 75e490b10b15a6a0422f25ff66ad98ef70390c8fecaac02712705dce8cc3564b
 2{
 3  "amount": -0.00583000,
 4  "fee": 0.00465000,
 5  "confirmations": 2177,
 6  "blockhash": "00000007567d87fadb4673d0cb876cb1b7b9dbfedc1d9a0e40af7380d9da8435",
 7  "blockheight": 104271,
 8  "blockindex": 1,
 9  "blocktime": 1660978126,
10  "txid": "75e490b10b15a6a0422f25ff66ad98ef70390c8fecaac02712705dce8cc3564b",
11  "wtxid": "e4b91ce21961a9e5498e2ff396f5e09fa8c7f3a36ef77abd4207f71855599990",
12  "walletconflicts": [
13  ],
14  "time": 1660976747,
15  "timereceived": 1660976747,
16  "bip125-replaceable": "no",
17  "details": [
18    {
19      "address": "tb1qkdeccjk32n0muv45xp67kq2pfd3qvfe6wym0ey",
20      "category": "send",
21      "amount": -0.00116600,
22      "vout": 4,
23      "fee": -0.00001400,
24      "abandoned": false
25    }
26  ],
27  "hex": "02000000000105b4c6a8258717bce1b92f761acf76324bb808b78879ff3ccead1452de1ad5bf0a0000000000ffffffffa8f3781e6929b82fbe8a4e821497932266a2c051b20324baba0d6ae33ffddbf00100000000ffffffff3105dc25c8bd51e921aecda35b35ca0f8ab7239f5d3452c969ce75731fcdc0f00100000000ffffffffc0c6b85c082753edfd21e9fe286f6b62dbfc6cb30d61a92667263f4057f02d170000000000fffffffff9338d880fab8b9904dbdad9c51e737b50ca670c3db9b8df195fb03e18d48cdd0100000000ffffffff0578c7010000000000160014b9861a8bf4af3ef5b96413d1c192912941e13ecc78c70100000000001600149cd3f14ce8775b771875d0826460e9255455aa8e78c70100000000001600143d264a7bfc3a3d3eaa205f9684be301da1729a3778c7010000000000160014cc74af1625a4f581dcd16bcb80ad9367958bf50d78c7010000000000160014b3738c4ad154dfbe32b43075eb01414b6206273a02473044022038884ba02e77e3c53c62412be065352b8c96b2420e117816e16b24fd6d9c47b202204257aad3ae0c9e42930ad2bb024b5adcda6f876c95b3772ae2ffa72a28327382012102449be5fb74725255eeeb50eba930fa87705f21e99d13cd710cf2c1f21153c8080247304402200a29d109643ddfb04fd26503c7e9887c9eabdc5f0ecc5072a357b91cd669ee8b022060c211a7c33eebcee308c641988e8cc67dbbe3b65856d4c69a2501a8694b0187012102131ef894942f0f9fe8f3455084c17f010549a7702bbfd74e4dff489738d0827f024730440220433cc9d973edfc3def7ec6afc49e18551e6a8af3267b75b6b72081bb80f99bbe022022473db462bcb6b698928c27fbda8bfe616cab07a62090e7dd69c4dd3a872523012103ac5d971ae3625f58c3ef5bcac46825653882bbb6a466f84d7226489dd876a1510247304402201f84bd62ab8e9986834da1f4e5e68c65d32cae7c6e2de69aa79666a53ea3696c022071a187d7248814080474e7c3a3fbbf771158f1e1c50e7d4e3c31a633f3af53fe012103025dc12c2024927ccbacb7f383d9a1a7d0709aaf73c7b22b794b82927f3be81a02473044022042636fb3f212af504b3196f27669c213342996f798aae6f50e4da0349f44c13e022076ce03586a0b6e387f330f6ef2ffc2fa952fca8c1391e0f68287ac9e17ff8381012103459928810bb656190ec36ca492f1e9c9ed84cc6ae09a0d2e9c62168bee2745b800000000"
28}
  14. luke-jr commented at 9:45 pm on September 3, 2022: member
    Then you provided the wrong metadata, because that looks right to me?
  15. ghost commented at 10:53 pm on September 3, 2022: none

    Then you provided the wrong metadata, because that looks right to me?

    I think it was correct output but i need a break for a few days before reviewing anything

  16. in src/wallet/rpc/spend.cpp:1497 in 10bbb0a525 outdated 
    1492+
1493+            RPCTypeCheck(request.params, {
1494+                UniValue::VSTR, // hexstring
1495+                UniValue::VOBJ, // options
1496+            }, /*fAllowNull=*/true);
1497+
    amovfx commented at 5:36 pm on September 10, 2022:
    Would this PR be of interest to you? https://github.com/bitcoin/bitcoin/pull/26039
  17. amovfx commented at 5:46 pm on September 10, 2022: none

    This was an excellent pr for the review club. I learned a ton from this.

    Is this worth testing with the legacy wallet?

  18. in src/wallet/transaction.h:254 in 10bbb0a525 outdated 
    247+                if (m_foreign_outputs.at(i)) {
248+                    s[i / 8] |= 1 << (i % 8);
249+                }
250+            }
251+            mapValueCopy["fout"] = s;
252+        }
    amovfx commented at 0:37 am on September 12, 2022:
    I am really looking forward to learning what this does. For the life of me I can’t figure out what you are doing with the string or why this works.
    amovfx commented at 9:44 pm on September 12, 2022:
    I got an answer on bitcoin stack exchange.
  19. in src/wallet/rpc/spend.cpp:1516 in 10bbb0a525 outdated 
    1511+                if (!options["foreign_outputs"].isNull()) {
1512+                    auto output_count = tx->vout.size();
1513+                    foreign_outputs.emplace(output_count);
1514+                    const auto& foreign_outputs_uv = options["foreign_outputs"].get_array();
1515+                    size_t final_foreign_outputs_size = 0;
1516+                    for (auto i = foreign_outputs_uv.size(); i--; ) {
    maflcko commented at 9:46 am on September 12, 2022:
    node0 stderr wallet/rpc/spend.cpp:1516:63: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type ‘size_t’ (aka ‘unsigned long’)
    luke-jr commented at 9:10 pm on September 12, 2022:
    AFAIK this is defined behaviour and correct.
    luke-jr commented at 11:59 pm on September 16, 2022:

    This one didn’t need the index at all, so changed it.

    Opened https://github.com/llvm/llvm-project/issues/57794 to address the false positives directly, and added exceptions for the others for now.

    sipa commented at 0:16 am on September 17, 2022:

    This is intentional. We pass in -fsanitize=integer which explicitly enables runtime warnings for well-defined, but commonly undesirable, behavior.

    I consider this mostly a shortcoming of the C/C++ languages. It only has signed types (which are used to represent mathematical integers, but with the restriction that the programmer guarantees they never go outside a certain range), and unsigned types (which are used to represent mathematical integers modulo a power of two). Ideally, there would also be unsigned types without modulo behavior but range restriction instead. This lack of distinction means the best a sanitizer for detecting undesirable (but not UB) behavior do is conservatively assume the modulo behavior was never intended, and need suppressions for the cases where it is.

  20. in src/wallet/transaction.h:245 in 10bbb0a525 outdated 
    240@@ -231,6 +241,15 @@ class CWalletTx
241         if (nTimeSmart) {
242             mapValueCopy["timesmart"] = strprintf("%u", nTimeSmart);
243         }
244+        if (!m_foreign_outputs.empty()) {
245+            std::string s(m_foreign_outputs.size(), 0);
    LarryRuane commented at 4:27 am on September 14, 2022: 
    0            std::string s((m_foreign_outputs.size() + 7) / 8, 0);
  21. in test/functional/wallet_listtransactions.py:320 in 10bbb0a525 outdated 
    315+        # node 1 thinks it received 2 BTC, then sent 2 BTC + 47 BTC change + 1 BTC with a -47 BTC fee
316+        txlist_node0 = self.nodes[0].listtransactions('*', 5)
317+        txlist_node1 = self.nodes[1].listtransactions('*', 5)
318+        assert txlist_node0.pop(0)['txid'] != txid
319+        assert txlist_node1.pop(0)['txid'] != txid
320+        txlist = list((tx['address'], tx['amount'], tx['category'], tx.get('fee', None)) for tx in txlist_node0 + txlist_node1)
    stickies-v commented at 10:53 am on September 14, 2022: 
    0        txlist = [(tx['address'], tx['amount'], tx['category'], tx.get('fee', None)) for tx in txlist_node0 + txlist_node1]
    luke-jr commented at 9:58 pm on September 16, 2022:
    I think explicitly using list(...) is clearer ([...] seems to work, but it’s not obvious that it isn’t just making a list with a single generator element)
  22. luke-jr force-pushed on Sep 14, 2022
  23. crimscim approved
  24. in test/functional/wallet_listtransactions.py:358 in 1697c21711 outdated 
    353+            ])
354+
355+        final_check()
356+
357+        # Ensure metadata is preserved across restart
358+        self.nodes[0].args = ["valgrind"] +self.nodes[0].args
    stickies-v commented at 10:43 am on September 15, 2022:

    Should this be in here or is this leftover from debugging? It doesn’t seem to work, did you mean to set self.options.valgrind = True instead? The current version is failing the CI:

     02022-09-14T18:45:53.166000Z TestFramework (ERROR): Unexpected exception caught during testing
 1Traceback (most recent call last):
 2  File "/private/var/folders/xx/vl5f934s6k927z1vyyl9cxth0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/test_framework/test_framework.py", line 133, in main
 3    self.run_test()
 4  File "/private/var/folders/xx/vl5f934s6k927z1vyyl9cxth0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/wallet_listtransactions.py", line 111, in run_test
 5    self.run_coinjoin_metadata_test()
 6  File "/private/var/folders/xx/vl5f934s6k927z1vyyl9cxth0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/wallet_listtransactions.py", line 359, in run_coinjoin_metadata_test
 7    self.restart_node(0)
 8  File "/private/var/folders/xx/vl5f934s6k927z1vyyl9cxth0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/test_framework/test_framework.py", line 576, in restart_node
 9    self.start_node(i, extra_args)
10  File "/private/var/folders/xx/vl5f934s6k927z1vyyl9cxth0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/test_framework/test_framework.py", line 533, in start_node
11    node.start(*args, **kwargs)
12  File "/private/var/folders/xx/vl5f934s6k927z1vyyl9cxth0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/test_framework/test_node.py", line 213, in start
13    self.process = subprocess.Popen(self.args + extra_args, env=subp_env, stdout=stdout, stderr=stderr, cwd=cwd, **kwargs)
14  File "/Applications/Xcode-13.3.0.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8/lib/python3.8/subprocess.py", line 858, in __init__
15    self._execute_child(args, executable, preexec_fn, close_fds,
16  File "/Applications/Xcode-13.3.0.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.8/lib/python3.8/subprocess.py", line 1704, in _execute_child
17    raise child_exception_type(errno_num, err_msg, err_filename)
18FileNotFoundError: [Errno 2] No such file or directory: 'valgrind'
    luke-jr commented at 10:20 pm on September 16, 2022:
    Accident leftover from debugging (I couldn’t get --valgrind to work at all). Removed.
  25. in test/functional/wallet_listtransactions.py:352 in 1697c21711 outdated 
    347+                # node 0:
348+                (addr_pay0, 1, 'receive', None),
349+                (addr_pay1, -2, 'send', -1),
350+                # node 1:
351+                (addr_pay1, 2, 'receive', None),
352+                (addr_pay0, -1, 'send', -2),
    stickies-v commented at 10:43 am on September 15, 2022:

    From a test perspective I think both should be equivalent, but conceptually in a CoinJoin situation (and how it was outlined in the example earlier, at least to my understanding), wouldn’t each node mark all the other node’s outputs (both send and change) as foreign?

     0        self.nodes[0].submitrawtransactiontowallet(rawtx, {'foreign_outputs': (2, 3)})
 1        self.nodes[1].submitrawtransactiontowallet(rawtx, {'foreign_outputs': (0, 1)})
 2
 3        def final_check():
 4            # Now node 0 should show it received 1 BTC, then sent 1 BTC with a 2 BTC fee
 5            # Now node 1 should show it received 2 BTC, then sent 2 BTC with a 1 BTC fee
 6            txlist_node0 = self.nodes[0].listtransactions('*', 3)
 7            txlist_node1 = self.nodes[1].listtransactions('*', 3)
 8            assert txlist_node0.pop(0)['txid'] != txid
 9            assert txlist_node1.pop(0)['txid'] != txid
10            txlist = list((tx['address'], tx['amount'], tx['category'], tx.get('fee', None)) for tx in txlist_node0 + txlist_node1)
11            assert_equal(txlist, [
12                # node 0:
13                (addr_pay0, 1, 'receive', None),
14                (addr_pay0, -1, 'send', -2),
15                # node 1:
16                (addr_pay1, 2, 'receive', None),
17                (addr_pay1, -2, 'send', -1),
    luke-jr commented at 10:01 pm on September 16, 2022:
    They’re paying each other, not themselves.
  26. in src/wallet/receive.cpp:207 in 1697c21711 outdated 
    201@@ -202,11 +202,7 @@ void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx,
202 
203     // Compute fee:
204     CAmount nDebit = CachedTxGetDebit(wallet, wtx, filter);
205-    if (nDebit > 0) // debit>0 means we signed/sent this transaction
206-    {
207-        CAmount nValueOut = wtx.tx->GetValueOut();
    stickies-v commented at 10:47 am on September 15, 2022:
    Perhaps helpful mentioning in the commit message that the omission of MoneyRange() by inlining it is safe because it’s already done in the line just above it (CAmount nDebit = CachedTxGetDebit(wallet, wtx, filter);)?
    luke-jr commented at 10:06 pm on September 16, 2022:
    I don’t see where CachedTxGetDebit calls MoneyRange on outputs, but in any case, it’s not a useful check here.
    stickies-v commented at 9:12 pm on September 19, 2022:
    CachedTxGetDebit calls GetCachableAmount which calls GetDebit which calls MoneyRange.
    luke-jr commented at 10:15 pm on September 19, 2022:
    GetDebit works with inputs, not outputs.
  27. in src/wallet/rpc/spend.cpp:1481 in 1697c21711 outdated 
    1476+        },
1477+        RPCResult{RPCResult::Type::NONE, "", ""},
1478+        RPCExamples{
1479+            "\nCreate a transaction\n"
1480+            + HelpExampleCli("createrawtransaction", "\"[{\\\"txid\\\" : \\\"mytxid\\\",\\\"vout\\\":0}]\" \"{\\\"myaddress\\\":0.01}\"") +
1481+            "Sign the transaction, and get back the hex\n"
    stickies-v commented at 10:56 am on September 15, 2022:

    Slightly inconsistent spacing (see output in details below), I think you want to add one \n here?

    0            "\nSign the transaction, and get back the hex\n"

     0...
 1Examples:
 2
 3Create a transaction
 4> bitcoin-cli createrawtransaction "[{\"txid\" : \"mytxid\",\"vout\":0}]" "{\"myaddress\":0.01}"
 5Sign the transaction, and get back the hex
 6> bitcoin-cli signrawtransactionwithwallet "myhex"
 7
 8Send the transaction (signed hex)
 9> bitcoin-cli submitrawtransactiontowallet "signedhex"
10
11As a JSON-RPC call
12> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "submitrawtransactiontowallet", "params": ["signedhex"]}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
13...
    luke-jr commented at 10:20 pm on September 16, 2022:
    Done
  28. in src/wallet/rpc/spend.cpp:1506 in 1697c21711 outdated 
    1501+            }
1502+            CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
1503+
1504+            std::optional<std::vector<bool>> foreign_outputs;
1505+            if (!request.params[1].isNull()) {
1506+                const UniValue& options = request.params[1];
    stickies-v commented at 11:29 am on September 15, 2022: 
    0                const UniValue& options{request.params[1]};
  29. in src/wallet/rpc/spend.cpp:1512 in 1697c21711 outdated 
    1507+                RPCTypeCheckObj(options, {
1508+                    {"foreign_outputs", UniValueType(UniValue::VARR)},
1509+                }, /*fAllowNull=*/true, /*fStrict=*/true);
1510+
1511+                if (!options["foreign_outputs"].isNull()) {
1512+                    auto output_count = tx->vout.size();
    stickies-v commented at 11:29 am on September 15, 2022:

    Generally recommended to use {} list initialization, I won’t highlight all instances here to not spam too many comments

    0                    auto output_count{tx->vout.size()};
    luke-jr commented at 10:09 pm on September 16, 2022:
    I don’t really care, so fine, but IMO it’s less readable. Is there a reason for it, that should perhaps get documented?
  30. stickies-v commented at 12:32 pm on September 15, 2022: contributor

    Did a first code review already. If we want to track foreign outputs, this approach makes sense to me.

    I’m wondering, would it be more user-friendly to instead track non-foreign outputs? For a user that is only using Core to create transactions, we could automatically track all outputs created internally and assume everything that’s not in that set is foreign. This way, the user would not have to submit any additional input for listtransactions to properly work? And could still allow users to manually set outputs to be non-foreign, similar to what’s implemented now? I’m not very familiar with the wallet, so apologies if I’m missing something obvious.

    I think it would also be nice to update the PR description with a bit more context on what the importance of this PR is, i.e. what is the impact of the current non-sensical behaviour. Are there functional implications (I’m not sure what the general purpose of listtransactions is)?

  31. Wallet: Refactor CachedTxGetAmounts fee calculation to inline value_out 
    This effectively inline CTransaction::GetValueOut

(Bypassed MoneyRange check is not relevant here)
    202eca15b6
  32. RPC/Wallet: Add submitrawtransactiontowallet method 42c040bc2d
  33. luke-jr force-pushed on Sep 16, 2022
  34. ghost commented at 11:30 pm on September 16, 2022: none
    I took a break. Although I think this PR needs to consider what is a “conjoin transaction”
  35. luke-jr force-pushed on Sep 16, 2022
  36. in test/sanitizer_suppressions/ubsan:58 in 9594807d96 outdated 
    52@@ -53,6 +53,9 @@ unsigned-integer-overflow:policy/fees.cpp
53 unsigned-integer-overflow:prevector.h
54 unsigned-integer-overflow:script/interpreter.cpp
55 unsigned-integer-overflow:txmempool.cpp
56+unsigned-integer-overflow:wallet/transaction.cpp
57+unsigned-integer-overflow:wallet/transaction.h
58+unsigned-integer-overflow:wallet/wallet.cpp
    maflcko commented at 6:57 am on September 17, 2022:

    not sure if we want to suppress three whole files for something that can also be fixed by moving one statement down a line.

    for(auto i{a.size()}; i--;) { ... } becomes for(auto i{a.size()}; i;) {--i; ... }

    luke-jr commented at 2:05 pm on September 19, 2022:
    Done
  37. bitcoin deleted a comment on Sep 18, 2022
  38. bitcoin deleted a comment on Sep 18, 2022
  39. bitcoin deleted a comment on Sep 18, 2022
  40. bitcoin deleted a comment on Sep 18, 2022
  41. bitcoin deleted a comment on Sep 18, 2022
  42. Wallet: Add a way to write modified CWalletTx 9405c54dd8
  43. luke-jr force-pushed on Sep 19, 2022
  44. ghost commented at 5:02 pm on September 19, 2022: none

    I took a break. Although I think this PR needs to consider what is a “conjoin transaction”

    Sorry for sharing less details. There are 3 main coinjoin implementations available but they have different type of transactions and its easy to spot on-chain. Does this PR work for all coinjoin that do not follow any of these implementations?

  45. Wallet: Extend wallet flag bitmask to support arbitrary named flags 29591fa797
  46. Wallet: Ensure caches are marked dirty in CWalletTx::Init 
    Since m_foreign_outputs can affect amounts, we need to clear the cache
    fe986906a9
  47. luke-jr force-pushed on Sep 19, 2022
  48. luke-jr commented at 9:58 pm on September 19, 2022: member

    There are 3 main coinjoin implementations available but they have different type of transactions and its easy to spot on-chain. Does this PR work for all coinjoin that do not follow any of these implementations?

    This PR is neutral about how you create the transaction. It just provides a way to tell this wallet about the details needed to display it correctly (and acts on that, for RPC).

  49. luke-jr commented at 9:59 pm on September 19, 2022: member
    Added a wallet flag to prevent old software versions from trying to load wallet files with foreign-output metadata. (Currently a one-way flag: it isn’t possible to unset, even if you remove foreign-output metadata later - it would need to scan your entire wallet to know to remove it)
  50. luke-jr requested review from stickies-v on Sep 19, 2022
  51. luke-jr removed review request from stickies-v on Sep 19, 2022
  52. luke-jr requested review from maflcko on Sep 19, 2022
  53. luke-jr removed review request from maflcko on Sep 19, 2022
  54. luke-jr requested review from LarryRuane on Sep 19, 2022
  55. luke-jr removed review request from LarryRuane on Sep 19, 2022
  56. luke-jr requested review from stickies-v on Sep 19, 2022
  57. Wallet: Add m_foreign_outputs to CWalletTx (stored in mapValue internally) 90328e7810
  58. Wallet: Ignore foreign outputs in fee/send amounts ca528669c9
  59. RPC/Wallet: Enable assigning foreign_outputs via submitrawtransactiontowallet method 2ab1547b2d
  60. QA: wallet_listtransactions: Extend coinjoin test to get correct results using foreign_outputs metadata da87dacaec
  61. luke-jr force-pushed on Sep 19, 2022
  62. achow101 requested review from achow101 on Apr 25, 2023
  63. DrahtBot commented at 12:52 pm on May 1, 2023: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.

  64. DrahtBot added the label Needs rebase on May 1, 2023
  65. in src/wallet/rpc/spend.cpp:1477 in da87dacaec 
    1472+                        },
1473+                    },
1474+                },
1475+            },
1476+        },
1477+        RPCResult{RPCResult::Type::NONE, "", ""},
    maflcko commented at 11:10 am on May 2, 2023:
    For new RPCs it might be good to return an empty dict to avoid breaking changes later on and confusing behavior now.
  66. DrahtBot commented at 1:09 am on July 31, 2023: contributor

    There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

    • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
    • Is it no longer relevant? ➡️ Please close.
    • Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
  67. achow101 commented at 4:09 pm on September 20, 2023: member
    Closing as up for grabs due to lack of activity.
  68. achow101 closed this on Sep 20, 2023
  69. achow101 added the label Up for grabs on Sep 20, 2023
  70. bitcoin locked this on Sep 19, 2024


luke-jr ghost DrahtBot amovfx maflcko sipa LarryRuane stickies-v crimscim achow101


achow101 stickies-v

Labels
Wallet Up for grabs Needs rebase

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me