polkadot-js / api

Promise and RxJS APIs around Polkadot and Substrate based chains via RPC calls. It is dynamically generated based on what the Substrate runtime provides in terms of metadata.
Apache License 2.0
1.06k stars 342 forks source link

somthing wrong in bitvec codec #5886

Open mrtnetwork opened 1 month ago

mrtnetwork commented 1 month ago

In the BitVec codec, I see the following comment:

 * @name BitVec
 * @description
 * A BitVec that represents an array of bits. The bits are however stored encoded. The difference between this
 * and a normal Bytes would be that the length prefix indicates the number of bits encoded, not the bytes
 */

However, in the decoding section, we have:

function decodeBitVecU8a (value?: Uint8Array): [number, Uint8Array] {
  if (!value?.length) {
    return [0, new Uint8Array()];
  }

  // handle all other Uint8Array inputs, these do have a length prefix which is the number of bits encoded
  const [offset, length] = compactFromU8aLim(value);
  const total = offset + Math.ceil(length / 8);

  if (total > value.length) {
    throw new Error(`BitVec: required length less than remainder, expected at least ${total}, found ${value.length}`);
  }

  return [length, value.subarray(offset, total)];
}

Can you please double-check the encoding and decoding logic for BitVec? Thanks!

TarikGul commented 1 month ago

Do you mind explaining what exactly is wrong with this implementation?

mrtnetwork commented 1 month ago

Do you mind explaining what exactly is wrong with this implementation?

The current implementation of the BitVec class contains a discrepancy in how the length is encoded. According to the class comment, the length prefix should indicate the number of bits encoded, not the number of bytes.

/**
 * A BitVec that represents an array of bits. The bits are however stored encoded. The difference between this
 * and a normal Bytes would be that the length prefix indicates the number of bits encoded, not the bytes.
 */

The method responsible for calculating the encoded length currently returns:

return this.length + compactToU8a(this.#decodedLength).length;

The length should be encoded correctly as the number of bits. Therefore, the correct implementation should be:

return this.length + compactToU8a(this.#decodedLength * 8).length;

decode section


function decodeBitVecU8a (value?: Uint8Array): [number, Uint8Array] {
  if (!value?.length) {
    return [0, new Uint8Array()];
  }

  // handle all other Uint8Array inputs, these do have a length prefix which is the number of bits encoded
  const [offset, length] = compactFromU8aLim(value);
  const total = offset + Math.ceil(length / 8);

  if (total > value.length) {
    throw new Error(`BitVec: required length less than remainder, expected at least ${total}, found ${value.length}`);
  }

  return [length, value.subarray(offset, total)];
}

/** @internal */
function decodeBitVec (value?: AnyU8a): [number, Uint8Array] {
  if (Array.isArray(value) || isString(value)) {
    const u8a = u8aToU8a(value);

    return [u8a.length / 8, u8a];
  }

  return decodeBitVecU8a(value);
}

constructor (registry: Registry, value?: AnyU8a, isMsb = false) {
    const [decodedLength, u8a] = decodeBitVec(value);

    super(registry, u8a);

    this.#decodedLength = decodedLength;
    this.#isMsb = isMsb;
  }