SChernykh / p2pool

Decentralized pool for Monero mining
GNU General Public License v3.0
1.07k stars 128 forks source link

Wrong merge mining tag #249

Closed moneromooo-monero closed 9 months ago

moneromooo-monero commented 1 year ago

In block_template.cpp:

    m_minerTxExtra.push_back(TX_EXTRA_MERGE_MINING_TAG);
    writeVarint(HASH_SIZE, m_minerTxExtra);
    m_minerTxExtra.insert(m_minerTxExtra.end(), HASH_SIZE, 0);

This writes 32 and a hash. This is very likely because it is meant to write a 32 byte string. This is not the format of the merge mining tag. From Monero's tx_extra.h:

  BEGIN_SERIALIZE()
    VARINT_FIELD_N("depth", mm_tag.depth)
    FIELD_N("merkle_root", mm_tag.merkle_root)
  END_SERIALIZE()
};

size_t depth;
crypto::hash merkle_root;

Depth is a varint, which is usually 0. crypto::hash is not a string, but a fixed length field, so its length is not written in the bitstream. It happens to be a valid merge mining tag with depth 32, but by chance.

Additionally, in block_template.cpp again:

    // Calculate sidechain id with this extra_nonce
    const hash sidechain_id = calc_sidechain_hash(extra_nonce);
    const size_t sidechain_hash_offset = extra_nonce_offset + m_poolBlockTemplate->m_extraNonceSize + 2;

This code assumes the depth varint is one byte (which is the case for both 32 and 0 in practice).

SChernykh commented 1 year ago

I'm aware, and this can't be fixed without another P2Pool fork. P2Pool should've used another tx_extra tag for this data (sidechain id).

SChernykh commented 9 months ago

WIP here: https://github.com/SChernykh/p2pool/tree/merge-mining