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

Possible to generate a block that is too large #1136

Closed
abitmore opened this Issue Jul 13, 2018 · 6 comments

Comments

Projects
2 participants
@abitmore
Member

abitmore commented Jul 13, 2018

This one: steemit/steem#2632.

IMHO, BitShares doesn't have the same issue, since BitShares block header is fixed size so far. There are 2 fields in Steem block header which made its size non-fixed, witness (string) and extensions (array-like), see changes in the PR steemit/steem#2637.

I'm creating this ticket so other developers will get notified, and hope they'll review the code as well. If we have the issue, we need to fix it. If we're going to add data to extensions in block header in the future, need to take care of related code to avoid similar issues.

@abitmore

This comment has been minimized.

Member

abitmore commented Jul 13, 2018

If we're going to add data to extensions in block header in the future, need to take care of related code to avoid similar issues.

To remember this, I think we can add a note to the code

struct block_header
{
digest_type digest()const;
block_id_type previous;
uint32_t block_num()const { return num_from_id(previous) + 1; }
fc::time_point_sec timestamp;
witness_id_type witness;
checksum_type transaction_merkle_root;
extensions_type extensions;
static uint32_t num_from_id(const block_id_type& id);
};

Why not port code from Steem now? Because current code has slightly better performance in current situation due to static:

static const size_t max_block_header_size = fc::raw::pack_size( signed_block_header() ) + 4;

abitmore added a commit that referenced this issue Jul 16, 2018

@abitmore abitmore added this to To Do in Feature Release (201808) via automation Jul 16, 2018

@abitmore abitmore moved this from To Do to In Testing in Feature Release (201808) Jul 16, 2018

abitmore added a commit that referenced this issue Jul 21, 2018

Merge pull request #1142 from bitshares/1136-block-size-note
Added a comment to block_header struct #1136

@abitmore abitmore closed this Jul 21, 2018

Feature Release (201808) automation moved this from In Testing to Done Jul 21, 2018

@abitmore

This comment has been minimized.

Member

abitmore commented Jul 21, 2018

Closed via #1142.

@abitmore abitmore changed the title from Steem block size issue to Block size issue Aug 3, 2018

@abitmore abitmore changed the title from Block size issue to Possible to generate a block that is too large Aug 3, 2018

@abitmore abitmore added this to To do in Feature release (201810) via automation Aug 3, 2018

@abitmore abitmore removed this from Done in Feature Release (201808) Aug 3, 2018

@abitmore

This comment has been minimized.

Member

abitmore commented Aug 3, 2018

Just noticed that witness_id_type in block header is not fixed-length after serialized. Reopening this issue for further discussion and action.

@abitmore abitmore reopened this Aug 3, 2018

@jmjatlanta

This comment has been minimized.

jmjatlanta commented Aug 3, 2018

My notes (preliminary, verify before trusting):

block_header has a few variable-width fields. witness_id_type and extensions_type. I believe extensions_type is not being used. If it is, I believe a block can easily be created that exceeds max_block_header_size.

Issue #1088 will increase the size of signed_block_header, so max_block_header_size should also return a larger (although not always accurate) size.

@abitmore

This comment has been minimized.

Member

abitmore commented Aug 13, 2018

According to this in-code comment, it can cause unexpected large blocks as well:

size_t new_total_size = total_block_size + fc::raw::pack_size( tx );
// postpone transaction if it would make block too big
if( new_total_size >= maximum_block_size )
{
postponed_tx_count++;
continue;
}
try
{
auto temp_session = _undo_db.start_undo_session();
processed_transaction ptx = _apply_transaction( tx );
temp_session.merge();
// We have to recompute pack_size(ptx) because it may be different
// than pack_size(tx) (i.e. if one or more results increased
// their size)
total_block_size += fc::raw::pack_size( ptx );

E.G. force settlement can return different size depends on whether the asset is globally settled.

I think we should check size before merging the undo session.

@abitmore

This comment has been minimized.

Member

abitmore commented Aug 16, 2018

Fixed by #1252.

@abitmore abitmore closed this Aug 16, 2018

Feature release (201810) automation moved this from To do to Done Aug 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment