Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

txprepare destinationAddress:all slow 1 "[\"yourOutpoint\"]" sends everything to the change address #4258

Closed
gabridome opened this issue Dec 4, 2020 · 5 comments · Fixed by #4259
Assignees

Comments

@gabridome
Copy link
Contributor

gabridome commented Dec 4, 2020

Issue and Steps to Reproduce

I have tried to use txprepare to send the whole amount of one UTXO to and address of my personal wallet.

I have used the syntax on the subject:

lightning-cli txprepare myPersonalWalletAddress:all slow 1 "[\"nodeOutpoint\"]"

{
   "unsigned_tx": "0200000......",
   "txid": "cd5..."
}
bitcoin-cli decoderawtransaction 020000...
{
  "txid": "cd5...",
  "hash": ".....",
  "version": 2,
  "size": 82,
  "vsize": 82,
  "weight": 328,
  "locktime": 659783,
  "vin": [
    {
      "txid": "fkkb...",
      "vout": 1,
      "scriptSig": {
        "asm": "",
        "hex": ""
      },
      "sequence": 4294967293
    }
  ],
  "vout": [
    {
      "value": 0.0029888929,
      "n": 0,
      "scriptPubKey": {
        "asm": "0 8...",
        "hex": "00...",
        "reqSigs": 1,
        "type": "witness_v0_keyhash",
        "addresses": [
          "bc1...." <<<<<<<<<<<<<<<<<<<<< not myPersonalWalletDestinationAddress
        ]
      }
    }
  ]
}

The destination address in the output is one of the change addresses of my node.

getinfo output

{
   "id": "02....",
   "alias": "mynode",
   "color": "27ckhs",
   "num_peers": 0,
   "num_pending_channels": 0,
   "num_active_channels": 0,
   "num_inactive_channels": 0,
   "address": [
      {
         "type": "torv3",
         "address": "onionaddress.onion",
         "port": 9735
      }
   ],
   "binding": [
      {
         "type": "ipv4",
         "address": "127.0.0.1",
         "port": 9735
      }
   ],
   "version": "v0.9.2",
   "blockheight": 659883,
   "network": "bitcoin",
   "msatoshi_fees_collected": 838,
   "fees_collected_msat": "838msat",
   "lightning-dir": "/home/username/.lightning/bitcoin"
}
cdecker added a commit to cdecker/lightning that referenced this issue Dec 4, 2020
We were not checking that outputs is indeed an array, and just going
ahead creating the array of outputs. Since `tok->size` for a string is
0 we ended up ignoring the argument altogether and thus the created
transaction would end up only with a single change output.

Fixes ElementsProject#4258
@cdecker cdecker self-assigned this Dec 4, 2020
@cdecker
Copy link
Member

cdecker commented Dec 4, 2020

I have reproduced this and fixed it in a branch: https://github.com/cdecker/lightning/tree/issue-4258

Just want to check whether this also solves #4238, since the broken array syntax looks pretty much the same.

@gabridome
Copy link
Contributor Author

gabridome commented Dec 4, 2020

So, if I understand this well, the problem is due to the fact:

  1. That I didn't enclose the output address and the amount i in square brackets
  2. That the software, instead of giving a parsing error, has sent the whole amount to a change address.

The right syntax should be:

lightning-cli txprepare '[{"myPersonalWalletAddress":"all"}]" slow 1 '["nodeOutpoint"]'

Writing this just anyone who maybe experiencing the same problem.

cdecker added a commit to cdecker/lightning that referenced this issue Dec 4, 2020
@cdecker
Copy link
Member

cdecker commented Dec 4, 2020

The right syntax should be:

lightning-cli txprepare "[\"myPersonalWalletAddress:all\"]" slow 1 "[\"nodeOutpoint\"]"

Writing this just anyone who maybe experiencing the same problem.

I'm afraid that is also not quite correct, though with #4259 this would get caught and would return an error message. I allowed myself to fix up your example, so future users can find it easily.

The syntax requires the outputs to be a list of dicts, and each dict has a single address as key, and an amount as the corresponding value. We might change that some time soon to remove one nesting (dict of addresses to amounts seems like a good option and corresponds to the format that sendtomany for bitcoin accepts).

@cdecker
Copy link
Member

cdecker commented Dec 4, 2020

If you're interested, the fix is in 4274985:

Malformed arguments would get interpreted as a zero-length array (tok->type == JSMN_STRING -> tok->size = 0) and therefore we would create a tx with 0 desired outputs, leaving just the change output as observed.

@gabridome
Copy link
Contributor Author

gabridome commented Dec 4, 2020

so:

lightning-cli txprepare "[{\"myPersonalWalletAddress:all\"}]" slow 1 "[\"nodeOutpoint\"]"

or

lightning-cli txprepare "[{\"myPersonalWalletAddress\":\"all\"}]" slow 1 "[\"nodeOutpoint\"]"

?
I would opt for the second.
Anyway, better to wait for the patch to be merged or the next release right?

cdecker added a commit to cdecker/lightning that referenced this issue Dec 4, 2020
cdecker added a commit to cdecker/lightning that referenced this issue Dec 5, 2020
cdecker added a commit to cdecker/lightning that referenced this issue Dec 5, 2020
cdecker added a commit to cdecker/lightning that referenced this issue Dec 7, 2020
We were not checking that outputs is indeed an array, and just going
ahead creating the array of outputs. Since `tok->size` for a string is
0 we ended up ignoring the argument altogether and thus the created
transaction would end up only with a single change output.

Fixes ElementsProject#4258
cdecker added a commit to cdecker/lightning that referenced this issue Dec 7, 2020
cdecker added a commit to cdecker/lightning that referenced this issue Dec 7, 2020
We were not checking that outputs is indeed an array, and just going
ahead creating the array of outputs. Since `tok->size` for a string is
0 we ended up ignoring the argument altogether and thus the created
transaction would end up only with a single change output.

Fixes ElementsProject#4258
cdecker added a commit to cdecker/lightning that referenced this issue Dec 7, 2020
cdecker added a commit to cdecker/lightning that referenced this issue Dec 7, 2020
We were not checking that outputs is indeed an array, and just going
ahead creating the array of outputs. Since `tok->size` for a string is
0 we ended up ignoring the argument altogether and thus the created
transaction would end up only with a single change output.

Fixes ElementsProject#4258
cdecker added a commit to cdecker/lightning that referenced this issue Dec 7, 2020
cdecker added a commit to cdecker/lightning that referenced this issue Dec 7, 2020
We were not checking that outputs is indeed an array, and just going
ahead creating the array of outputs. Since `tok->size` for a string is
0 we ended up ignoring the argument altogether and thus the created
transaction would end up only with a single change output.

Fixes ElementsProject#4258
cdecker added a commit to cdecker/lightning that referenced this issue Dec 7, 2020
cdecker added a commit to cdecker/lightning that referenced this issue Dec 8, 2020
We were not checking that outputs is indeed an array, and just going
ahead creating the array of outputs. Since `tok->size` for a string is
0 we ended up ignoring the argument altogether and thus the created
transaction would end up only with a single change output.

Fixes ElementsProject#4258
cdecker added a commit to cdecker/lightning that referenced this issue Dec 8, 2020
rustyrussell pushed a commit that referenced this issue Dec 8, 2020
We were not checking that outputs is indeed an array, and just going
ahead creating the array of outputs. Since `tok->size` for a string is
0 we ended up ignoring the argument altogether and thus the created
transaction would end up only with a single change output.

Fixes #4258
vibhaa pushed a commit to spider-pcn/lightning that referenced this issue Mar 24, 2021
We were not checking that outputs is indeed an array, and just going
ahead creating the array of outputs. Since `tok->size` for a string is
0 we ended up ignoring the argument altogether and thus the created
transaction would end up only with a single change output.

Fixes ElementsProject#4258
vibhaa pushed a commit to spider-pcn/lightning that referenced this issue Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants