Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to serialize merkle block with transactions #881

Merged
merged 2 commits into from
Oct 17, 2019

Conversation

braydonf
Copy link
Contributor

This adds the ability to serialize the merkle block data structure to include the transactions, which would otherwise be omitted in the process. There are cases where it's necessary to serialize and persist a merkle block before the block is sent to the wallet.

@codecov-io

This comment has been minimized.

@pinheadmz
Copy link
Member

Changes look good to me, test passes and coverage is good.

Wondering about MerkleBlock.format() and .getJSON() with respect to the value txs. In a full block, both functions return a txs value that is a JSON array of each tx also JSONified.

In merkleblock, format()returns the number of txs, and getJSON() does not return txs at all. I think behavior of MerkleBlock should match that of Block, especially now that we are adding transactions to it.

@braydonf
Copy link
Contributor Author

To include the transactions for getJSON, I think there would be a matching getExtendedJSON as the transactions are not included in default serialization, and there is a matching fromJSON(). I don't think that is really necessary at this point.

Added txs to format() to match that of block.

braydonf added a commit that referenced this pull request Oct 17, 2019
Add option to serialize merkle block with transactions
@braydonf braydonf merged commit 3b24b16 into bcoin-org:master Oct 17, 2019
@braydonf braydonf deleted the merkle-serialize branch October 17, 2019 20:17
@braydonf braydonf added this to the 2.0.0 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants