fix: operate on block header only #24
Conversation
| @@ -6,6 +6,8 @@ const multihashes = require('multihashes') | |||
| const multihashing = require('multihashing-async') | |||
| const waterfall = require('async/waterfall') | |||
|
|
|||
| const BITCOIN_BLOCK_HEADER_SIZE = 80 | |||
There was a problem hiding this comment.
Is it standard practice to SHOUT CONSTANTS?
There was a problem hiding this comment.
Good question, I'm used to shout constants. I couldn't find any pointers to a standard, but I'm used to do that in all the languages I code.
Codecov Report
@@ Coverage Diff @@
## master #24 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 3 3
Lines 83 84 +1
=====================================
+ Hits 83 84 +1
Continue to review full report at Codecov.
|
| .util.BITCOIN_BLOCK_HEADER_SIZE | ||
|
|
||
| const headerFromHexBlock = (hex) => { | ||
| return Buffer.from(hex.toString(), 'hex').slice(0, BITCOIN_BLOCK_HEADER_SIZE) |
There was a problem hiding this comment.
Seems inefficient in time and memory. What is the hex type? Why the toString()?
If hex is a string, then it would be better to first take the BITCOIN_BLOCK_HEADER_SIZE * 2 characters and then create the Buffer from it.
If this method is just used by tests, then you can ignore the above.
There was a problem hiding this comment.
I'ts just for tests. The Bitcoin blocks are serialized as "hex", it's bytes encoded as hex put into a file. That's what the Go implementation is using and i didn't want to convert the files, but just use the same ones for the JS implementation as well.
|
@richardschneider The actual change is pretty subtle. Things worked accidentally. Prior to this change you could put a full Bitcoin block (which consists of the header and transaction) into IPLD and the tests would pass. Though this is wrong. Using only the header is the correct way. So this change is mostly changing the tests so that they only use the header of the block. It will make more sense, once we switch the |
The current code was wrong. You could pass in a full block (with transactions) and only the header was used. The correct behaviour is, to use the header only. This means that this library does support block headers only, there is no support for transactions.
|
@richardschneider Have I cleared up your concerns? If yes, please approve this PR. |
The current implementation was wrong. You were able to add a full
block (including transactions), but the CID was only calculated
based on the header.
The correct implementation is to work on the block header only.
Transaction support needs to be implemented separately.