Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Update ship's file format #7113

Merged
merged 9 commits into from
Apr 12, 2019
Merged

Update ship's file format #7113

merged 9 commits into from
Apr 12, 2019

Conversation

tbfleming
Copy link
Contributor

@tbfleming tbfleming commented Apr 11, 2019

Change Description

Replace ship's header to address these concerns in #5970:

  • Switch to serialization instead of memcpy
  • Rework header versioning
  • Drop block_num since it's included in block_id
  • Check header version in recover_blocks

These changes, plus changes in other PRs, make ship files incompatible with previous nodeos versions.

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

@tbfleming tbfleming marked this pull request as ready for review April 11, 2019 15:33
fc::datastream<char*> ds(bytes, sizeof(bytes));
fc::raw::pack(ds, header);
EOS_ASSERT(!ds.remaining(), chain::plugin_exception, "state_history_log_header_serial_size mismatch");
log.write(bytes, sizeof(bytes));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though it should be the same in this case, it is better use ds.tellp() rather than sizeof(bytes) here.

log.seekg(0, std::ios_base::end);
uint64_t pos = log.tellg();
log.write((char*)&header, sizeof(header));
write_header(header);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could store the position right after writing the header but before writing the payload:

uint64_t payload_start_pos = log.tellg();

write_payload(log);
uint64_t end = log.tellg();
EOS_ASSERT(end == pos + sizeof(header) + header.payload_size, chain::plugin_exception,
EOS_ASSERT(end == pos + state_history_log_header_serial_size + header.payload_size, chain::plugin_exception,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then here the condition could be end == payload_start_pos + header.payload_size.

if (suffix + sizeof(header) + header.payload_size + sizeof(suffix) != size) {
read_header(header, false);
if (!is_ship(header.magic) || !is_ship_supported_version(header.magic) ||
suffix + state_history_log_header_serial_size + header.payload_size + sizeof(suffix) != size) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last part of condition could be log.tellg() + header.payload_size + sizeof(suffix) != size.

uint64_t suffix;
if (header.payload_size > size || pos + sizeof(header) + header.payload_size + sizeof(suffix) > size)
if (!is_ship(header.magic) || !is_ship_supported_version(header.magic) || header.payload_size > size ||
pos + state_history_log_header_serial_size + header.payload_size + sizeof(suffix) > size) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last part of condition could be log.tellg() + header.payload_size + sizeof(suffix) > size.

break;
log.seekg(pos + sizeof(header) + header.payload_size);
}
log.seekg(pos + state_history_log_header_serial_size + header.payload_size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace with log.seekg( log.tellg() + header.payload_size ).

log.read((char*)&suffix, sizeof(suffix));
if (suffix != pos)
break;
pos = pos + sizeof(header) + header.payload_size + sizeof(suffix);
pos = pos + state_history_log_header_serial_size + header.payload_size + sizeof(suffix);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace with pos = log.tellg().

EOS_ASSERT(header.version == 0 && sizeof(header) + header.payload_size + sizeof(uint64_t) <= size,
read_header(header, false);
EOS_ASSERT(is_ship(header.magic) && is_ship_supported_version(header.magic) &&
state_history_log_header_serial_size + header.payload_size + sizeof(uint64_t) <= size,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last part of condition could be log.tellg() + header.payload_size + sizeof(uint64_t) <= size.

log.read((char*)&header, sizeof(header));
uint64_t suffix_pos = pos + sizeof(header) + header.payload_size;
read_header(header, false);
uint64_t suffix_pos = pos + state_history_log_header_serial_size + header.payload_size;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace with uint64_t suffix_pos = log.tellg() + header.payload_size;.

log.seekg(suffix_pos);
log.read((char*)&suffix, sizeof(suffix));
// ilog("block ${b} at ${pos}-${end} suffix=${suffix} file_size=${fs}",
// ("b", header.block_num)("pos", pos)("end", suffix_pos + sizeof(suffix))("suffix", suffix)("fs", size));
EOS_ASSERT(suffix == pos, chain::plugin_exception, "corrupt ${name}.log (8)", ("name", name));

state_history_summary summary{.pos = pos};
index.write((char*)&summary, sizeof(summary));
index.write((char*)&pos, sizeof(pos));
pos = suffix_pos + sizeof(suffix);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also just be pos = log.tellg();.

@arhag
Copy link
Contributor

arhag commented Apr 11, 2019

We will skip the above changes for now since it only goes partially towards removing the dependency on the state_history_log_header serializing to a fixed size. In the future, if state_history_log_header needs to be able to serialize to a variable-length byte serialization (for example to support variants), it will require more changes than the ones suggested here and so we will likely have to revisit the existing structure of the code at that time anyway.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants