pooler / cpuminer

CPU miner for Litecoin and Bitcoin
https://bitcointalk.org/index.php?topic=55038.0
Other
2.77k stars 1.21k forks source link

Bug or not? #121

Closed mutluit closed 7 years ago

mutluit commented 7 years ago

Isn't there a bug in creating the block header?

For example "work->data[17] = swab32(curtime);" is used in the below excerpt. But shouldn't that be done only on big endian machines? My reasoning is this: The following example code does not convert the time (also nor version, nor bits), and stilll gets the right answer: https://pastebin.com/raw/bW3fQA2a Therefore, maybe cpuminer does it wrong?

Same/similar argumentation for the fields "version" and "bits".

Also, in scanhash_sha256_xxx() the following is used memcpy(data, pdata + 16, 64); But aren't the offset 16 and len 64 wrong? Shouldn't the offset be 0 and len 80?

cpu-miner.c ... / assemble block header / work->data[0] = swab32(version); for (i = 0; i < 8; i++) work->data[8 - i] = le32dec(prevhash + i); for (i = 0; i < 8; i++) work->data[9 + i] = be32dec((uint32_t *)merkle_tree[0] + i); work->data[17] = swab32(curtime); work->data[18] = le32dec(&bits); ...

pooler commented 7 years ago

Short answer: probably not. This code has been tested and used to mine valid blocks on various testnets. The probability that this kind of bug could have gone unnoticed seems extremely low.

But shouldn't that be done only on big endian machines?

It all depends on the byte order you decide to use internally. cpuminer keeps header data in host-endian form, so the code you quoted makes sense to me.

But aren't the offset 16 and len 64 wrong? Shouldn't the offset be 0 and len 80?

No. This is the second 64-byte block of data. The first block is used to compute the midstate.

mutluit commented 7 years ago

But the function calls swab32(), le32dec(), and be32dec() are called unconditionally for all machine types regardless of their endian type. I think the program is not limited to just one endian type machine, or is it?

pooler commented 7 years ago

That's because the endianness of the data provided by GBT does not depend on the endianness of the machine. The program should work correctly on both little- and big-endian systems.

mutluit commented 7 years ago

But GBT (ie. the server) does not know what endianness the remote client has. I assume the server always delivers the data in just one endian format, ie. in one standard format. It is then the duty of the client to know its own endianness and convert the received data accordingly if it needs to, depending on how the algorithm works on the data. So, for a little endian and a big endian machine there should be differences in how they process the recived raw data in accordance with how their hashing algo works. Therefore, for me it's hard to imagine that cpuminer code does exactly the same operations on both big endian and little endian machines.

pooler commented 7 years ago

The data is provided by the server as JSON, not in binary form.

mutluit commented 7 years ago

Ok, I did debug prints of the "version" and the "curtime" fields:

work->data[0] = swab32(version); applog(LOG_DEBUG, "version=0x%0x(%u) work->data[0]=0x%0x(%u)", version, version, work->data[0], work->data[0]);

work->data[17] = swab32(curtime); applog(LOG_DEBUG, "curtime=0x%0x(%u) work->data[17]=0x%0x(%u)", curtime, curtime, work->data[17], work->data[17]);

It prints: version=0x20000002(536870914) work->data[0]=0x2000020(33554464) curtime=0x5937fb39(1496841017) work->data[17]=0x39fb3759(972765017)

But, according to the specification these two fields in the blockhdr have to be in little endian format. Since my machine is little endian, I would have expected that work->data[0] be the same as the content of the variable version, but which is not, due to the unconditional swab32()... Same with "curtime".

And: you are taking the "version" from the GBT. But is such a huge version number like 536870914 correct at all when mining blocks? I was expecting 1, 2, 3, or 4 according to https://bitcoin.org/en/developer-reference#block-versions

Isn't it? :-)

pooler commented 7 years ago

I can understand your confusion. I said earlier that the data is kept in host-endian form, but I should have been more precise: it is kept in reverse-host-endian order. The reason for this is that while the Bitcoin protocol requires little-endian encoding, SHA-256 processes data as 32-bit big-endian integers.

mutluit commented 7 years ago

Hmm. but as shown in the pastebin example, there is, on a little endian machine, no swab32() used with the version and curtime fields, eventhough it of course too applies twice sha256 on the data block.

Is there no test code in cpuminer to verify its correct working, for example by regenerating the genesis block or any other known block?

And how do you explain the use of such a huge version number? (cf. prev. posting) Will a submitted solution with such a huge version number not automatically be rejected as invalid?

FYI: I had many false positives in the last 2 weeks running cpuminer (ie. minerd). But what if they actually weren't false positives... :-)

pooler commented 7 years ago

Hmm. but as shown in the pastebin example, there is, on a little endian machine, no swab32() used with the version and curtime fields, eventhough it of course too applies twice sha256 on the data block.

That's because your code uses a different SHA-256 implementation. Most implementations expect bytes as input and do the decoding themselves; the one in cpuminer takes alredy decoded 32-bit integers.

Is there no test code in cpuminer to verify its correct working, for example by regenerating the genesis block or any other known block?

There are no public tests, but if you don't trust me you can easily verify the code's correctness by mining a block on some testnet. I mined some myself just a few weeks ago to test Segwit support (on Litecoin's testnet v4).

pooler commented 7 years ago

I was about to close this due to inactivity when I noticed that you had edited your last post to add a couple questions, so let me answer those.

And how do you explain the use of such a huge version number? (cf. prev. posting)

Do you mean 0x20000002? It looks correct to me; you may need to look up BIP9.

FYI: I had many false positives in the last 2 weeks running cpuminer (ie. minerd).

It's expected to find many false positives on SHA-256d if the difficulty is higher than 1; that's a side-effect of how the algorithm is implemented. In the long run the ratio of true positives should be inversely proportional to the difficulty.

But what if they actually weren't false positives... :-)

In that case the problem wouldn't be the endianness. False positives are detected locally by comparing the result of the partial SHA-256d calculation to the result of a full implementation of the algorithm, and both functions use the same encoding.