bcoin-org / bcash

Implementation of Bitcoin Cash protocol in node.js
Other
102 stars 68 forks source link

Can't createTX() with OP_RETURN output #95

Closed gtklocker closed 6 years ago

gtklocker commented 6 years ago

The output is considered dust so the function throws, however a tx with an OP_RETURN is still a valid tx.

dionyziz commented 6 years ago

Can you try increasing its value?

bucko13 commented 6 years ago

Yeah, note that even OP_RETURNs should need to pay a fee.

gtklocker commented 6 years ago

Is this right? I mean there definitely will be a fee (on the whole transaction) but I've never seen the value of specifically the OP_RETURN output be something other than 0. Example: https://live.blockcypher.com/btc/tx/88f89cecc445d4e62a5d5c2ccbf6e512348b18ff2b9be23473d62f9660629846/

Also, createrawtransaction on Bitcoin-ABC when passed a data key will also create a tx with zero value on the OP_RETURN output, fundrawtransaction or signrawtransaction do not affect it. And it does get relayed/confirmed just fine.

OrfeasLitos commented 6 years ago

It seems like each output needs to be above the dust limit (which in Bitcoin Core with default parameters for a simple P2PKH turns out to be 546 shatoshis): https://www.reddit.com/r/Bitcoin/comments/2unzen/what_is_bitcoins_dust_limit_precisely/ https://github.com/bitcoin/bitcoin/commit/9022aa3

I couldn't find a definitive answer though.

gtklocker commented 6 years ago

Again, this is a confirmed Bitcoin transaction from 3 hours ago:

https://live.blockcypher.com/btc/tx/88f89cecc445d4e62a5d5c2ccbf6e512348b18ff2b9be23473d62f9660629846/ http://chainquery.com/bitcoin-api/getrawtransaction/88f89cecc445d4e62a5d5c2ccbf6e512348b18ff2b9be23473d62f9660629846/1

As you can see, the OP_RETURN output has 0 value:

...
            {
                "value": 0.00000000,
                "n": 0,
                "scriptPubKey": {
                    "asm": "OP_RETURN 6f6d6e69000000000000001f0000000c59684c18",
                    "hex": "6a146f6d6e69000000000000001f0000000c59684c18",
                    "type": "nulldata"
                }
            },
...
gtklocker commented 6 years ago

@OrfeasLitos with regards to the bitcoin source you cited, it can easily be seen that there's special treatment for non-spendable outputs. https://github.com/morcos/bitcoin/blob/f4d00e63f7951469c840edede3f675d07249f62a/src/policy/policy.cpp

CAmount GetDustThreshold(const CTxOut& txout, const CFeeRate& dustRelayFeeIn)
{
    if (txout.scriptPubKey.IsUnspendable())
        return 0;
...
}

bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFeeIn)
{
    return (txout.nValue < GetDustThreshold(txout, dustRelayFeeIn));
}

This would mean that IsDust on a 0-value OP_RETURN should evaluate to 0 < 0 which in turn is false.

OrfeasLitos commented 6 years ago

I think you found here the crucial spot. You can make a PR here if you have the time.

gtklocker commented 6 years ago

By the way, OP_RETURN outputs don't even go through the IsDust check: https://github.com/morcos/bitcoin/blob/f4d00e63f7951469c840edede3f675d07249f62a/src/policy/policy.cpp#L143-L144

The only limitation is, you can have only one such output.

gtklocker commented 6 years ago

@bucko13 any update on this? There's the exact same issue on bcoin, maybe we should open an issue there?

bucko13 commented 6 years ago

Sorry for the confusion. My point wasn't that the OP_RETURN output needs to have the fee since fees are basically paid by inputs, just the transaction as a whole that's creating it. I've personally sent OP_RETURNs with bcoin before (though with bcoin but this should be the same for both) so it should work and you're right that it shouldn't do a dust check, which I don't think it does. So my guess is that the problem is with the other output, the change, being dust.

This could be a bug with the MTX fund method picking a coin as an input that's too small for fee + change. More likely though there's something wrong with how the tx is being composed. Could you share a snippet of how you're creating the tx and what the JSON for the tx is?

P.S. if you're interested in seeing bcoin/bcash code that composes valid OP_RETURN outputs (including multiple outputs for 1 tx), there's a bpanel plugin that you can take a look at and play around with on your own node if you get bpanel setup: https://github.com/bpanel-org/publish-data

gtklocker commented 6 years ago

Here's a full minimal example which fails:

const bcash = require('bcash');
(async () => {
  const node = new bcash.SPVNode({});
  const {wdb} = node.use(bcash.wallet.plugin);
  await node.open();
  await node.connect();

  const wallet = wdb.primary;
  await wallet.send({
    outputs: [bcash.Script.fromNulldata(Buffer.from('foo'))]
  });
})().catch((err) => {
  console.error(err);
  process.exit(1);
});

Output:

Error: Output is dust.
    at Wallet.createTX (/home/vagrant/bch-nipopow-prover/node_modules/bcash/lib/wallet/wallet.js:1219:15)
    at Wallet._send (/home/vagrant/bch-nipopow-prover/node_modules/bcash/lib/wallet/wallet.js:1284:28)
    at Wallet.send (/home/vagrant/bch-nipopow-prover/node_modules/bcash/lib/wallet/wallet.js:1269:25)
    at process._tickCallback (internal/process/next_tick.js:68:7)
gtklocker commented 6 years ago

With regards to your hypothesis, as I stated earlier, the problem is in createTX (link to problematic code).

As you can see all outputs are tested for dustedness unconditionally, so this also checks the OP_RETURN which fails. At this point fund isn't called yet. This can also be seen from the stack trace above.

OrfeasLitos commented 6 years ago

Could the above fail because this wallet has no coins? You could try to give it some coins before sending the tx, taking inspiration from here: https://github.com/OrfeasLitos/TrustIsRisk.js/blob/56808e3805b026ab543eef6f26670308e55100fa/test/spv_node.js#L3, https://github.com/OrfeasLitos/TrustIsRisk.js/blob/56808e3805b026ab543eef6f26670308e55100fa/test/spv_node.js#L36, https://github.com/OrfeasLitos/TrustIsRisk.js/blob/56808e3805b026ab543eef6f26670308e55100fa/test/spv_node.js#L178-L182

gtklocker commented 6 years ago

No. I've ensured the wallet has enough balance, and can serve the transaction just fine. Creating the tx manually, funding and sending it does work.

OrfeasLitos commented 6 years ago

Could you also provide the minimal testcase that works as well? It will make comparison easier.

bucko13 commented 6 years ago

outputs should be an array of output objects. In your example script, you're just passing a Script class as an item on the array which send won't know how to interpret. Luckily the Output class in bcoin/bcash has a fromScript method you can use to convert your OP_RETURN script, as seen in the bpanel plugin mentioned above (link to relevant code here).

gtklocker commented 6 years ago

You're right.