bitcoinjs / bitcoinjs-lib

A javascript Bitcoin library for node.js and browsers.
MIT License
5.69k stars 2.11k forks source link

`RangeError: value out of range` parsing LTC mweb tx #1886

Closed fboucquez closed 1 year ago

fboucquez commented 1 year ago

Hi team, thank you for providing and maintaining this library

It seems like Transaction.fromBuffer does not like LTC txs where MimbleWimble is involved.

Here's an example

https://live.blockcypher.com/ltc/tx/7643f12e323af0eb326a8b104f3f3d94f4bd8bc4f052aeecfa09bf2096c092d5/

And this is a script to reproduce the error (note that pulling the rawtx from the web can be avoided by uncommenting the const rawtx )


var bitcoinjs = require("bitcoinjs-lib");
var Transaction = bitcoinjs.Transaction;
const get = url => {
  return new Promise((resolve, reject) => {
    const http = require("http");
    const https = require("https");
    let client = http;
    if (url.toString().indexOf("https") === 0) {
      client = https;
    }
    client
      .get(url, resp => {
        let data = "";

        resp.on("data", chunk => {
          data += chunk;
        });
        resp.on("end", () => {
          resolve(data);
        });
      })
      .on("error", err => {
        reject(err);
      });
  });
};

const test = async () => {
  // https://live.blockcypher.com/ltc/tx/7643f12e323af0eb326a8b104f3f3d94f4bd8bc4f052aeecfa09bf2096c092d5/

  const txId =
    "7643f12e323af0eb326a8b104f3f3d94f4bd8bc4f052aeecfa09bf2096c092d5";

  const apiUrl = `https://litecoin.a.exodus.io/insight/rawtx/${txId}`;
  const { rawtx } = JSON.parse(await get(apiUrl));
  console.log(rawtx);

  // const rawtx =
  //   '02000000000801b50597162ce0d744bb919b2e35582d44dab74e5beaa1cf13f98038faedd2b63a0000000000ffffffff02cd2f2147de0000002258206bc64dafa10f417509ef99f97ab3674d8ddb9fd0af800eb372aa8da0bc7fb18e20a10700000000001976a9149d3bc0d46a32c756b67f93b6f3896f5cfd71bbaa88ac0000000000'

  const tx = Transaction.fromBuffer(Buffer.from(rawtx, "hex"));
  console.warn("txId:", txId, "can be parsed, result:", tx);
};

test();

This raises an Error: RangeError: value out of range trying to parse the value of one of the tx.outs

I know this lib is not 100% supporting LTC, but I'm wondering if this is something that could be fixed.

In particular, we would like to include that tx as a nonWitnessUtxo when sending. Afaik, this requires parsing the tx (which currently fails)

junderw commented 1 year ago

It looks like MWEB transactions use an ADVANCE_TRANSACTION_FLAG of 0x08 instead of 0x01.

Due to this, our parsing logic is trying to parse the tx as non-segwit, and therefore failing (because MWEB is segwit formatted (as far as I can tell))

If someone were to fork our library, they would need to change the logic surrounding hasWitness to include 0x08 for the MWEB transaction.

https://github.com/bitcoinjs/bitcoinjs-lib/blob/v6.1.0/ts_src/transaction.ts#L84-L92

Unfortunately, this is outside the scope for this library. Our support of altcoins ends with what can be encoded in the Network object.

Thanks for bringing up the issue for others to search, though!

fboucquez commented 1 year ago

Thanks @junderw . Would it be possible to abstract out the hasWitness logic so devs can provide their own customization? Similar to ecc

junderw commented 1 year ago

That's the thing.

Currently, the only two binary formats that exist for transactions are segwit and non-segwit.

The point of having the 0x00 and 0x01 bytes there was to allow for a versioning to the binary format.

Abstracting out the logic for "hasWitness" is incorrect, because in the bitcoin protocol only 0x01 has segwit, this is not something that is "customizable."

I am not even sure if the 0x08 MWEB binary format is actually a 1:1 perfect match to Segwit serialization, so if you COULD somehow jank in 0x08 (by using ts-ignore to overwrite the Transaction static constant etc) it might not be correct.

It looks like segwit serialization to me from your one example, but I'm not a Litecoin dev and have no clue how the binary serialization for MWEB transactions works.


I was merely showing where someone making a fork would need to look and where they would need to change things.

It seems like Litecoin now has multiple binary serialization versions, so a fork might want to abstract that away in some manner.

junderw commented 1 year ago

ie. You made a mention of using it as a nonWitnessUtxo... the reason why we must parse it is because we need to get the normalized txid, which requires reorganizing the data within a segwit transaction.

There is a chance that the normalization to get the txid for an MWEB transaction is different from a segwit transaction. In which case your Psbt will still fail validation.

It might not fail, I don't know. But these are the kinds of issues you'll run into if you try to deal with MWEB with a library that is made for another coin without MWEB.

I would recommend someone who knows about Litecoin fork the project (or otherwise propose a way of abstraction that can make sense for Bitcoin too without breaking backwards compatibility.)

fboucquez commented 1 year ago

Hi @junderw ,

Thank you for the detailed explanation. I'm not an LTC dev either (and hardly a BTC dev). My current workaround is to just provide the script in witnessUtxo when trying to consume one of those mweb utxo/tx (like a segwit or traproot utxo). Example:

psbt.__CACHE.__UNSAFE_SIGN_NONSEGWIT = true
psbt.addInput({ hash: txId, index: vout, sequence,  witnessUtxo: { value, script: Buffer.from(script, 'hex') } } )

No previous rawTx parsing. The flag avoids the lib rejection. This seems to be working fine.

btw, hey @motorina0 !

fboucquez commented 1 year ago

I'll try to propose a generic solution when upgrading our bitcoinjs-lib forks