search

MetacoSA / NBitcoin

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

Add maxArraySize parameter for Transaction.GetHash to allow calculation of hash for bigger transactions #1005

Closed albracko closed 3 years ago

albracko commented 3 years ago

NBitcoin already has option to set the BitcoinStream.MaxArraySize field to a size greater than 1MB when creating the Transaction object from a byte[]. But if you are trying to calculate the hash of such transaction, an exception occurs since GetHash method defaults the MaxArraySize parameter to 1MB and you cannot override this value

NicolasDorier commented 3 years ago
  1. There can't be such array in bitcoin
  2. If it was possible, this would be problematic, as the memory heap would quickly give you OutOfMemory exception because of the difficulty for the garbage collector to find enough contiguous space.

Why do you need this?

albracko commented 3 years ago
  1. There can't be such array in bitcoin
  2. If it was possible, this would be problematic, as the memory heap would quickly give you OutOfMemory exception because of the difficulty for the garbage collector to find enough contiguous space.

Why do you need this?

  1. There can be such an array and there already is...for instance on Bitcoin SV chain (here's an example of tx with 2,8MB in size, https://whatsonchain.com/tx/b559312f5a749c9c7cad9bbe8ecb01d71a648e5a726187365dcd5a5ef11a08c0), so for this i would be able to load the Tx, but then unable to call GetHash() method, since it crashes
  2. Hmmm, don't know why there would be an issue with a tx or block bigger than 1MB, regarding memory. I'm using NBitcoin (which i must compliment of course, because it's a really good library) for quite some time now. I was even able to load a block with a size of 600+ MB, when setting MaxArraySize above the default value.
NicolasDorier commented 3 years ago

You can read the block because probably the memory is not fragmented as it would be in an app actually running in prod.

That said, given the behavior of Craig Wright toward Bitcoin developers, I will not change NBitcoin for BitcoinSV, nor will support BitcoinSV in the altcoins package.

albracko commented 3 years ago

Hmmm...i'm not aware of what Craig Wright is doing since i'm not interested in that stuff. Nor am i asking for any support for BSV in altcoins...i just gave an example from BSV chain since i found an example there...and it seems it was my mistake to do so, since i was not aware this link would become an issue for this PR. If BCH which also supports Txs bigger than 1MB had more of them i would have given an example from their chain...but it was faster to find one in BSV chain

I'm just interested in adding improvements to NBitcoin package so you have option for more flexibility if needed.

NicolasDorier commented 3 years ago

@albracko this limit is not about the size of transactions, but about the size of a arrays in the transaction. Even if BCH has bigger block, I doubt they allow byte array to be unbounded. This is an obvious vulnerability.

albracko commented 3 years ago

agree, my mistake when explaining it...it's not about the size of transactions, i was using big OP_RETURN data because it's easier to make a big tx this way, hence the array size was too small.

Well if it's an vulnerability for a blockchain, let that be the decision of the chain itself, i believe NBitcoin is not trying to solve any vulnerability issues that blockchains may have, it's rather a very handy .NET library for us developers that try to create some tools for instance...and when i was trying out bigger txs i bumped against this limitation, and since it was possible to raise this limit in case of blocks and the tx itself i found it strange it's not possible to do the same when you try to get the hash of such tx. I mean there are also other methods that have same issue, but i think they are not used that often...well i know i didn't need them yet (but if i will i might open another PR with an improvement).

I hope, in the future, crypto developers of various blockchains will get along better than they do now and this stance who likes and who dislikes who will not be an issue when trying to drive the boundaries further...

NicolasDorier commented 3 years ago

@albracko there is no possibility to get along with scammers.

Closing this one as I won't change this for BSV.

That said, another fundamental reason I don't want to add it: GetHash() should not have parameters. How to calculate the hash is already implicit, since a new block is created form the NetworkConsensusFactory (which depends on the altcoin).

In other words, it is possible for shitcoins to make their own implementation of Block. (See Litecoin for example) This mean that user of the library do not need to know the details of the shitcoin to use it.

That said, I won't add BSV to NBitcoin.Altcoins either. But for coins like BCH, if that is a problem, this is something that can be done.