MetacoSA / NBitcoin

Comprehensive Bitcoin library for the .NET framework.
MIT License
1.88k stars 847 forks source link

Add BlockHeader.Equals #191

Closed GSPP closed 6 years ago

GSPP commented 8 years ago

I'd like to contribute BlockHeader.Equals/GetHashCode and operators. The code base appears to have different styles for those. Is it OK if I use a different style that is the best that I know of?

NicolasDorier commented 8 years ago

I'm not sure it makes sense to have a BlockHeader.Equals/GetHashCode method. Also it has the risk of breaking current client code which relied on comparison by ref.

GSPP commented 8 years ago

That's true, it's a compat risk. On the other hand I cannot imagine code that would be broken by this. The comparison practically would only become true when it was false. Not the other way around. So all this might do is match a different but equivalent object. (Of course it's possible to break code. It's just that I cannot imagine how that code would look like.)

My use case is comparing deserialized data with Bitcoin Core RPC data.

Feel free to close this.

NicolasDorier commented 8 years ago

True that there is rather low chance of breaking something.

My use case is comparing deserialized data with Bitcoin Core RPC data.

What do you mean ?

GSPP commented 8 years ago

I'm trying to read out the blk* files. I want to make sure what I'm reading corresponds to what Bitcoin Core thinks is true. There could be orphan blocks and I am also seeing outright corrupt data in these files (which is not a Bitcoin Core bug at all). This is part of my endeavor to access block data in a high speed way. That's why I wanted to compare headers. I'm doing that now using GetHash().

NicolasDorier commented 8 years ago

NBitcoin have the BlockStore class which read the blocks from the blk* files in high speed way. What is the problem with comparing using GetHash() ?

GSPP commented 8 years ago

GetHash is not semantically expressive and it's slower.

I did not know BlockStore existed but it's not correct. See the linked issue for what corrupt data you might find. The only approach that I know works is scan for the magic number and try to deserialize.

NicolasDorier commented 8 years ago

@GSPP my BlockStore does not assume block ordering. You should try I already used the class quite a lot without issue.

GSPP commented 8 years ago

Can the class deal with finding <magic><length><magic> patterns in the file? I don't see how it can. My block files have that, possibly due to unexpected shutdown during syncing. The files do not guarantee contiguous blocks.

If it works for you that good but it will not work on all installations.

NicolasDorier commented 8 years ago

long time I did not tried, but I vaguely remember it was the case, I may be wrong.

NicolasDorier commented 8 years ago

I don't thing GetHash is so slow btw, for 1 000 000 comparison, it is not really the bottleneck. The bottleneck will be IO performance I think.

NicolasDorier commented 8 years ago

I was thinking, if we override Equals/GetHash etc... for BlockHeader, we'll actually need a fast hash function. Why now using SipHash ?

GSPP commented 8 years ago

The crypto fields should make the hash code automatically very high quality. I think cryptographic values do not even need SipHash although that's fast, too.

I did not mean to hash a serialized version. I just intended to use something like Hash(fld1) ^ Hash(fld2) ... with trivial hash functions. For arrays the project seems to have helpers already.

NicolasDorier commented 8 years ago

I think you should use Siphash is already implemented, so we can just reuse it, the problem with xor is (a xor b) = (b xor a)

NicolasDorier commented 6 years ago

Closing, open since last year. Can reopen if you feel working on it.