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

Add block info & ram delta to action_trace #5339

Merged
merged 48 commits into from
Sep 13, 2018
Merged

Add block info & ram delta to action_trace #5339

merged 48 commits into from
Sep 13, 2018

Conversation

heifner
Copy link
Contributor

@heifner heifner commented Aug 20, 2018

Resolves #5149

action_trace & transaction_trace changes:

  • Add block_num, block_time, producer_block_id, context_free to action_trace
  • Add block_num, block_time, producer_block_id to transaction_trace
  • Add ram_delta per account to action_trace

mongo_db_plugin chnages:

  • Update block_state with irreversible=true on irreversible signal
  • Add transaction_trace status to each action_trace
  • Insert transaction_trace before its action_traces

@heifner heifner changed the title Gh#5149 mongo Add block_num & block_time to action_trace Aug 20, 2018
@heifner heifner changed the title Add block_num & block_time to action_trace Add block info & ram delta to action_trace Aug 24, 2018
@@ -1520,6 +1527,11 @@ time_point controller::pending_block_time()const {
return my->pending->_pending_block_state->header.timestamp;
}

block_id_type controller::pending_producer_block_id()const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Return optional<block_id_type> instead.

@@ -83,6 +83,8 @@ struct pending_state {

controller::block_status _block_status = controller::block_status::incomplete;

block_id_type _producer_block_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Using optional<block_id_type> instead. Empty value for the case when _block_status == controller::block_status::incomplete.

@@ -22,6 +22,10 @@ namespace eosio { namespace chain {

uint64_t total_cpu_usage = 0; /// total of inline_traces[x].cpu_usage + cpu_usage
transaction_id_type trx_id; ///< the transaction that generated this action
uint32_t block_num = 0;
block_timestamp_type block_time;
block_id_type producer_block_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use optional<block_id_type> instead. Same for the transaction_trace below. Traces emitted from an incomplete block would leave the producer_block_id as empty.

Then later in mongo_db_plugin, avoid adding the action traces or transaction traces to the database if the producer_block_id is empty. This way traces from speculatively executed transactions are not included in the Mongo database which can avoid potential confusion for consumers of that database.

Due to forks, it could be possible for multiple incompatible action traces with the same block_num and trx_id to exist in the database. And if the producer double produces a block, even the block_time may not disambiguate the two action traces. Without a producer_block_id to disambiguate and determine if the action trace comes from an orphaned fork branching off of the blockchain, consumers of the Mongo DB database may be reacting to a stale action trace that never actually executed in the current blockchain.

It is better to avoid this potential confusion by not logging traces from speculative execution, i.e. emitted from an incomplete block. This means that traces will not be properly recorded in speculative read-mode, but users should not be using the mongo_db_plugin in that mode anyway. It would be good if the mongo_db_plugin refuses to record any traces, whether they have producer_block_id set or not, if the node is operating under speculative read-mode; or better yet, the mongo_db_plugin could refuse to run in speculative read-mode. By generally not logging traces with empty producer_block_id in the mongo_db_plugin, it does however have the benefit that even with any of the other read-modes other than speculative (for example head), you do not get unneeded and inaccurate traces from speculatively executed transactions.

@@ -74,6 +74,7 @@ namespace eosio { namespace chain {
vector<action_receipt> executed;
flat_set<account_name> bill_to_accounts;
flat_set<account_name> validate_ram_usage;
flat_map<account_name, int64_t> account_ram_delta; // reset for each action
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of explicitly resetting for each action, move this field to apply_context.

@@ -1388,7 +1395,7 @@ fork_database& controller::fork_db()const { return my->fork_db; }

void controller::start_block( block_timestamp_type when, uint16_t confirm_block_count) {
validate_db_available_size();
my->start_block(when, confirm_block_count, block_status::incomplete );
my->start_block(when, confirm_block_count, block_status::incomplete, block_id_type() );
Copy link
Contributor

Choose a reason for hiding this comment

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

With the change to optional<block_id_type>, the last argument of this function call would be optional<block_id_type>() instead of block_id_type().

@@ -365,6 +368,7 @@ namespace eosio { namespace chain {
if( ram_delta > 0 ) {
validate_ram_usage.insert( account );
}
account_ram_delta[account] += ram_delta;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of adding this line here, you can create a new wrapper function in apply_context:

void apply_context::add_ram_usage( account_name account, int64_t ram_delta ) {
   trx_context.add_ram_usage( account, ram_delta );
   account_ram_delta[account] += ram_delta;
}

Then all calls to transaction_context::add_ram_usage in eosio_contract.cpp and apply_context.cpp could be replaced with calls to apply_context::add_ram_usage.

// It is recommended to run mongo_db_plugin in read-mode = read-only.
//
if( !is_producer && !t->producer_block_id.valid() )
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows our nodeos_run_test.py --mongodb test to pass since it has the same nodeos as both a producer and running the mongo_db_plugin. I figured people running a single test node might find that useful.

@sim31
Copy link
Contributor

sim31 commented Sep 8, 2018

I think account_ram_delta will be a very useful field. So I'm looking forward for this getting merged. But I see currently each element in account_ram_delta is an array which represents a pair (account, delta). I noticed, it could make it kind of complicated to make queries for this kind of schema. Would it be possible to make it an object?

@heifner
Copy link
Contributor Author

heifner commented Sep 8, 2018

Not sure why array is more difficult than an object. Can you provide an example of what you would like that would be easier.

@sim31
Copy link
Contributor

sim31 commented Sep 10, 2018

Say, I want to get all actions where account A payed for RAM. Now I have to query array of arrays for that. I've tried in ways, which seemed straightforward to me first - they didn't work. I had to dig around on the internet to find out how to do that, but that's not the problem. The problem is that I've found in multiple places where people say that that's a bad document structure and there will be problems when trying to do aggregation for example. Here for example.

Now, let's say I want to create an index on the payer field, to make the query I mentioned above faster. Didn't have time to think about it much yet, but I have no idea how to do it for the current document structure. Would be straightforward if account_ram_delta would contain objects. Now I could only find this so far, which again suggests that it's not the best document structure.

I'm under impression mongodb expects objects to be used these kind of scenarios and it's probably optimized better if objects are used as well.

@heifner
Copy link
Contributor Author

heifner commented Sep 12, 2018

@sim31

	"account_ram_deltas" : [
		{
			"account" : "eosio",
			"delta" : -128
		},
		{
			"account" : "gyydsmbxguge",
			"delta" : 128
		}
	],

Does this work for you?

@heifner
Copy link
Contributor Author

heifner commented Sep 12, 2018

@arhag Ready for your review of changes.

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.

3 participants