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

Deep Mind off-chain ABI serializer & FC encoded hex output #9073

Merged
merged 7 commits into from
Dec 3, 2020

Conversation

maoueh
Copy link
Contributor

@maoueh maoueh commented May 11, 2020

Change Description

ABI serialization of binary data to JSON will be performed completely off-chain from now on. The output of heavy stuff (TransactionTrace, BlockState, Transaction) are now written as hexadecimal output using the EOSIO fc encoding binary protocol.

Moreover, on boot, nodeos will now dump all known ABIs of each account. Through standard deep mind data exchange protocol, we now dump all ABIs of all account on startup of nodeos. This is required now because the ABI serialization will be performed off-chain, and to achieve that correctly, we require existing ABI at startup time.

Not that I could remove some of the changes around ABI serialization made to support deep mind in the previous patch if you would like to (specialized maybe_to_variant_with_abi, getters to expose ABI serializer max time in apply_context, and other related stuff).

Note I just rebased our change against develop, I'm in the process of compiling it against develop now to check if everything compiles correctly.

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

abourget and others added 2 commits May 11, 2020 11:58
Through standard deep mind data exchange protocol, we now dump all ABIs
of all account on startup of `nodeos`. This is required now because the
ABI serialization will be perfomed offchain, and to achieve that correctly,
we require existing ABI at startup time.
ABI serialization of binary data to JSON will be performed completely off-chain
from now on. The output of heavy stuff (TransactionTrace, BlockState, Transaction)
are now written as hexadecimal output uisng the EOSIO fc encoding binary protocol.

# Conflicts:
#	CMakeLists.txt
#	libraries/chain/apply_context.cpp
#	release_tag.sh
auto idx = db.get_index<account_index>();
for (auto& row : idx.indices()) {
if (row.abi.size() != 0) {
fc_dlog(*dm_logger, "ABIDUMP ABI ${block_num} ${contract} ${abi}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll probably move the {block_num} output in the START call, it's always the same here so it's not that useful to have it right here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The {block_num} is now printed only once when starting the ABI dump process.

@@ -750,6 +750,24 @@ struct controller_impl {
// else no checks needed since fork_db will be completely reset on replay anyway
}

if (auto dm_logger = get_deep_mind_logger()) {
// FIXME: We should probably feed that from CMake directly somehow ...
fc_dlog(*dm_logger, "DEEP_MIND_VERSION 12");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not the best place for this, open to suggestion to where to move it. Used by the reader to determine which format it's about to read.

// FIXME: We should probably feed that from CMake directly somehow ...
fc_dlog(*dm_logger, "DEEP_MIND_VERSION 12");

fc_dlog(*dm_logger, "ABIDUMP START");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was suggested to use chain_plugin#startup to place the ABIs dump. However, the the end of startup happens too late when replaying from an existing blocks.log file. Maybe it should still be in startup, just at a better place.

// FIXME: We should probably feed that from CMake directly somehow ...
fc_dlog(*dm_logger, "DEEP_MIND_VERSION 12");

fc_dlog(*dm_logger, "ABIDUMP START");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note also that we decided against using a flag for the initial ABIs dump. Mainly because the operation is really fast and the actual ABI serialization cannot work properly without the ABIs dump, so we see no reason to make it behind a flag.

@@ -1123,9 +1123,11 @@ void chain_plugin::plugin_initialize(const variables_map& options) {

my->accepted_block_connection = my->chain->accepted_block.connect( [this]( const block_state_ptr& blk ) {
if (auto dm_logger = my->chain->get_deep_mind_logger()) {
auto packed_blk = fc::raw::pack(*blk);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When a shared_ptr is used, it adds a first optionality byte to the binary data. So I had to use this here, but I'm wondering if this creates a copy of the structure prior packing it, I think it does.

Is there a way to avoid it if it's the case? Can I cast somehow to a const block_state& type?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no copy of *blk before packing. Also, fc::raw::pack takes its argument as const T&.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So what's pointing to the shared_ptr gets cast as const ref, perfect thanks!

@@ -1197,7 +1201,8 @@ void chain_plugin::plugin_startup()
}

my->chain_config.reset();
} FC_CAPTURE_AND_RETHROW() }
} FC_CAPTURE_AND_RETHROW()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Useless change, I'll revert that.

@@ -1144,9 +1146,11 @@ void chain_plugin::plugin_initialize(const variables_map& options) {
my->applied_transaction_connection = my->chain->applied_transaction.connect(
[this]( std::tuple<const transaction_trace_ptr&, const signed_transaction&> t ) {
if (auto dm_logger = my->chain->get_deep_mind_logger()) {
auto packed_trace = fc::raw::pack(*std::get<0>(t));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here for the copy and const reference cast (for me when updating the PR).

@brianjohnson5972
Copy link
Contributor

@maoueh Where are you at with @swatanabe-b1 's comment fixes?

@maoueh
Copy link
Contributor Author

maoueh commented Jun 22, 2020

@brianjohnson5972 @swatanabe-b1 Was not a proper review, more an answer to a general question I was asking. Unless I'm mistaken here.

So, this is still waiting for a review.

Matthieu Vachon added 2 commits November 24, 2020 12:04
# Conflicts:
#	libraries/appbase
#	libraries/chainbase
#	libraries/fc
#	plugins/chain_plugin/chain_plugin.cpp
- All PERM_OP now has a permission_id on the line to identify the permission.
- Refactored DEEPMIND_VERSION to pass two arguments, major and minor version.
- Refactored ABIDUMP to dump block num only once and to print active global sequence.
@@ -647,6 +647,7 @@ namespace eosio { namespace chain {
if (auto dm_logger = control.get_deep_mind_logger()) {
event_id = STORAGE_EVENT_ID("${id}", ("id", gto.id));

auto packed_signed_trx = fc::raw::pack(trx);
Copy link
Contributor

Choose a reason for hiding this comment

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

packed_signed_trx materially different from packed_trx ? This looks like an unnecessary "round trip" with line 626 above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll double check, I remember some had subtle differences when packing not giving the exact same output format, specially related to signature. One was giving an output with signature the other not, but I don't remember if this one was problematic or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, it appears packed_trx would

  • potentially have signatures (relevant if it weren't a deferred transaction)
  • be potentially compressed (relevant if it weren't a deferred transaction)
  • represent the (un)?compressed transaction as a blob of data and contain some envelope information about the blob's format

by re-packing it it should always be uncompressed and only the transaction BUT i don't think it will have signatures.

packed_transaction::get_transaction() const returns a transaction not a signed_transaction. Also, thisis a deferred transaction which has no signatures. Please rename the variable to something like serialized_trx so that future readers do not expect this blob to contain signatures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In version 2.x of deep mind branch, this was outputting signature (for delayed pushed transaction which is the case here). You are right that is not a signed_trx anymore here, it's a plain transaction without the signature.

Now, I checked and using fc::to_hex(packed_trx.get_packed_transaction().data(), packed_trx.get_packed_transaction().size()) does not print signature neither. That would have been a correct change since we could have uncompress the transaction if it was compressed.

Now, I think my best way of handling it here is to also output the signatures directly, hoping they are not pruned which seems the case here.

This does not compile yet because error: no member named 'visit' in 'fc::reflector<const std::__1::vector<fc::crypto::signature, std::__1::allocator<fc::crypto::signature> > *>' when used in a mutable variable object for printing like this:

            fc_dlog(*dm_logger, "DTRX_OP PUSH_CREATE ${action_id} ${sender} ${sender_id} ${payer} ${published} ${delay} ${expiration} ${trx_id} ${trx} ${signatures}",
               ("action_id", get_action_id())
...
               ("signatures", packed_trx.get_signatures())
            );

I still have some holes in my understanding of the bigger serialization format around fc and EOSIO codebase, but I don't see yet why it complains like this. The fc::crypto::signature has specialized to_variant and from_variant overload as well as having a dedicated FC_REFLECT(fc::crypto::signature, (_storage) ) definition.

So maybe you could give me some hint here to find why it's not working.

Copy link
Contributor

Choose a reason for hiding this comment

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

You have two options here:

  1. maintain the course with your new serialization, you should dereference the pointer so that the mutable_variant_object receives a const std::vector<signature_type>& instead of a pointer.

  2. go back to the original and change it to:

auto packed_signed_trx = fc::raw::pack(packed_trx.to_packed_transaction_v0().get_signed_transaction());

which should give you what you were seeing in the 2.x version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tips.

I went with the original output and the extra hop of turning it into a v0 packed transaction. It was easier to go that route, specially since I would also needed to add context_free_data.

FC_REFLECT_ENUM( chainbase::environment::arch_t,
(ARCH_X86_64)(ARCH_ARM)(ARCH_RISCV)(ARCH_OTHER) )
FC_REFLECT(chainbase::environment, (debug)(os)(arch)(boost_version)(compiler) )
FC_REFLECT_ENUM(chainbase::environment::os_t,
Copy link
Contributor

Choose a reason for hiding this comment

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

It appears this PR nearly entirely reformats this file which is unacceptable. If there were relevant changes to this file please make them as tersely as possible otherwise ensure the PR does not mutate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, I always try to not include any formatting change, I assume it's the ouuppss note :) I'll revert all this for sure.

@b1bart b1bart merged commit f6f6c40 into EOSIO:develop Dec 3, 2020
@maoueh maoueh deleted the feature/offchain-abi-and-hex-output branch December 3, 2020 16:43
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.

5 participants