-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Conversation
Hmm... This looks like a simple boost multi_index just sitting in RAM with no regard to blockchain history. That would be volatile storage, and it won't be resilient to forking. Should this be stored in the database instead, so it's persistent and kept in sync through fork switches? |
Yes, it is a simplistic implementation. I was mostly wanting to get the plugins in there and then make the implementation actually correct. Do you want me to fix that before committing or is it ok to have a naive implementation for now(with further work coming)? The plugin also needs tracking based on scope/authorities which will really affect memory (I think, since multi_index doesn't support matching on contents in a key that is a vector), needs to add filtering AccountNames (to only track accounts you care about to limit size) and also thinking that the plugin will need to just copy the contents of the block and store it in the database on another thread to prevent blocking the chain production. |
(Excuse my accidental closing of the pull request) I'll make the change to using the database to hold the multi_index, at the time I was holding off all the complexities, but the complexities are not there yet. |
Yes, please implement with chainbase. |
fdaeaf3
to
a251af2
Compare
@nathanhourt - I presume that github sends you an email when something is added to the pull request, but just in case, I figured I would manually send this. |
@brianjohnson5972 there were conflicts between this and Phil's work, can you please resolve them so that I can merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor stylistic recommendations
> | ||
>; | ||
|
||
typedef chainbase::generic_index<transaction_history_multi_index> transaction_history_index; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically we keep the object and index definitions in a header file. Please move to a header for consistency
@@ -171,6 +171,7 @@ namespace eos { namespace chain { | |||
transaction_object_type, | |||
producer_object_type, | |||
chain_property_object_type, | |||
transaction_history_object_type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment here noting that this object is defined in the account_history_plugin would be good (like in the lines below). With modern IDEs, it's not as necessary, but it's still helpful to note which object types are declared here, but implemented in a separate library/plugin.
@@ -171,7 +171,7 @@ namespace eos { namespace chain { | |||
transaction_object_type, | |||
producer_object_type, | |||
chain_property_object_type, | |||
transaction_history_object_type, | |||
transaction_history_object_type, ///< Defined by account history plugin library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Defined by account_history_plugin"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One gripe about exceptions; fix that and the rest looks good to me.
msg += "block was not found"; | ||
else | ||
msg += "transaction was not found in the block"; | ||
BOOST_THROW_EXCEPTION( std::runtime_error( msg ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use a std::runtime_error
-- we want to exclusively use our own exception types in this codebase.
This is throwing for a pretty weird case that, theoretically, should never actually occur, so we don't need to define an exception type for it specifically. We can just use assertions. Something like this should suffice:
FC_ASSERT(block, "Transaction with ID ${tid} was indexed as being in block ID ${bid}, but no such block was found", ("tid", transaction_id")("bid", block_id));
FC_THROW("Transaction with ID ${tid} was indexed as being in block ID ${bid}, but was not found in that block", ("tid", transaction_id")("bid", block_id));
@@ -171,7 +171,7 @@ namespace eos { namespace chain { | |||
transaction_object_type, | |||
producer_object_type, | |||
chain_property_object_type, | |||
transaction_history_object_type, ///< Defined by account history plugin library | |||
transaction_history_object_type, ///< Defined by account_history_plugin library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize, I have been unclear. This comment claims the object is defined by a plugin library, but it's not a library, it's a plugin. The purpose of the comment is to make it easier to find the object without an IDE tracking references of transaction_history_object_type
, so if it says "library" and "plugin", it's not obvious whether to look in the plugins
or libraries
directory. This comes from a plugin, so it should only say plugin and not library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I apologize for not paying as close of attention to your original comment.
{ | ||
const auto& db = chain_plug->chain().get_database(); | ||
optional<block_id_type> block_id; | ||
db.with_read_lock( [&]() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not compile:
../../plugins/account_history_plugin/account_history_plugin.cpp:42:7: error: no matching member function for call to 'with_read_lock'
db.with_read_lock( [&]() {
~~~^~~~~~~~~~~~~~
../../libraries/chainbase/include/chainbase/chainbase.hpp:881:15: note: candidate function not viable: 'this' argument has type 'const chainbase::database', but method is not marked const
auto with_read_lock( Lambda&& callback, uint64_t wait_micro = 1000000 ) -> decltype( (*(Lambda*)nullptr)() )
^
db is a const ref, but with_read_lock
is not a const method. I wonder... should with_read_lock
be const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I "fixed" that (Otherwise Travis would complain). see commit d48ebab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you able to get it to work, or is there an issue with my commit? I had a heck of a time getting the submodule commit correct, so it would not be a big shocker if it really didn't work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, just an old submodule. Oops :]
Hello, I caught the same error:
|
That means that you need to run "git submodule update". with_read_lock was changed to be const, but since it is a submodule it doesn't get updated when you do your git fetch or pull. |
It's didn't update with "git submodule update". But the same result (error) after force update
|
Sorry, you are correct, the submodule got changed to an earlier revision. We will get that fixed. |
Ok. Please let me know, when I can try build it one more time. |
What is on master now should build. |
It's ok. Thanx ) |
rename Eos
# This is the 1st commit message: various improvements # This is the commit message #2: new hash # This is the commit message #3: fix for script path # This is the commit message #4: fixes # This is the commit message #5: fixes # This is the commit message #6: fixes # This is the commit message #7: fixes # This is the commit message #8: fixes # This is the commit message #9: fixes # This is the commit message #10: fixes # This is the commit message #11: fixes # This is the commit message #12: fixes # This is the commit message #13: fixes # This is the commit message #14: fixes # This is the commit message #15: fixes # This is the commit message #16: fixes # This is the commit message #17: fixes # This is the commit message #18: fixes # This is the commit message #19: fixes # This is the commit message #20: fixes # This is the commit message #21: fixes # This is the commit message #22: fixes # This is the commit message #23: fixes # This is the commit message #24: fixes # This is the commit message #25: fixes # This is the commit message #26: testing # This is the commit message #27: testing # This is the commit message #28: testing # This is the commit message #29: testing # This is the commit message #30: testing # This is the commit message #31: testing # This is the commit message #32: testing # This is the commit message #33: testing # This is the commit message #34: testing # This is the commit message #35: testing # This is the commit message #36: testing # This is the commit message #37: testing # This is the commit message #38: testing # This is the commit message #39: testing # This is the commit message #40: testing # This is the commit message #41: testing # This is the commit message #42: testing # This is the commit message #43: testing # This is the commit message #44: fixes # This is the commit message #45: fixes # This is the commit message #46: fixes # This is the commit message #47: fixes # This is the commit message #48: fixes # This is the commit message #49: fixes # This is the commit message #50: fixes # This is the commit message #51: fixes # This is the commit message #52: fixes # This is the commit message #53: fixes # This is the commit message #54: fixes # This is the commit message #55: fixes # This is the commit message #56: fixes # This is the commit message #57: fixes # This is the commit message #58: fixes # This is the commit message #59: fixes # This is the commit message #60: fixes # This is the commit message #61: fixes # This is the commit message #62: fixes # This is the commit message #63: fixes # This is the commit message #64: fixes # This is the commit message #65: fixes # This is the commit message #66: fixes # This is the commit message #67: fixes # This is the commit message #68: fixes # This is the commit message #69: fixes # This is the commit message #70: fixes # This is the commit message #71: fixes # This is the commit message #72: fixes # This is the commit message #73: fixes # This is the commit message #74: fixes # This is the commit message #75: fixes # This is the commit message #76: fixes # This is the commit message #77: fixes # This is the commit message #78: fixes # This is the commit message #79: more testing # This is the commit message #80: testing # This is the commit message #81: fixes # This is the commit message #82: fixes # This is the commit message #83: fixes # This is the commit message #84: fixes # This is the commit message #85: fixes # This is the commit message #86: fixes # This is the commit message #87: fixes # This is the commit message #88: fixes # This is the commit message #89: fixes # This is the commit message #90: fixes # This is the commit message #91: fixes # This is the commit message #92: fixes # This is the commit message #93: propagate-environment for buildkite-agent # This is the commit message #94: propagate-environment for buildkite-agent # This is the commit message #95: propagate-environment for buildkite-agent # This is the commit message #96: propagate-environment for buildkite-agent # This is the commit message #97: fixes # This is the commit message #98: fixes # This is the commit message #99: fixes # This is the commit message #100: fixes # This is the commit message #101: fixes # This is the commit message #102: fixes # This is the commit message #103: fixes # This is the commit message #104: fixes # This is the commit message #105: fixes # This is the commit message #106: fixes # This is the commit message #107: fixes # This is the commit message #108: fixes # This is the commit message #109: fixes # This is the commit message #110: fixes # This is the commit message #111: fixes # This is the commit message #112: fixes # This is the commit message #113: fixes # This is the commit message #114: fixes # This is the commit message #115: fixes # This is the commit message #116: fixes # This is the commit message #117: fixes # This is the commit message #118: fixes # This is the commit message #119: fixes # This is the commit message #120: fixes # This is the commit message #121: fixes # This is the commit message #122: fixes # This is the commit message #123: fixes # This is the commit message #124: fixes # This is the commit message #125: fixes # This is the commit message #126: fixes # This is the commit message #127: fixes # This is the commit message #128: fixes # This is the commit message #129: fixes # This is the commit message #130: fixes # This is the commit message #131: fixes # This is the commit message #132: fixes # This is the commit message #133: fixes # This is the commit message #134: fixes # This is the commit message #135: fixes # This is the commit message #136: fixes # This is the commit message #137: fixes # This is the commit message #138: fixes # This is the commit message #139: fixes # This is the commit message #140: fixes # This is the commit message #141: fixes # This is the commit message #142: fixes # This is the commit message #143: fixes # This is the commit message #144: fixes # This is the commit message #145: fixes # This is the commit message #146: fixes # This is the commit message #147: fixes # This is the commit message #148: fixes # This is the commit message #149: fixes # This is the commit message #150: fixes # This is the commit message #151: fixes # This is the commit message #152: fixes # This is the commit message #153: testing # This is the commit message #154: fixes # This is the commit message #155: fixes # This is the commit message #156: fixes # This is the commit message #157: fixes # This is the commit message #158: fixes # This is the commit message #159: fixes # This is the commit message #160: fixes # This is the commit message #161: fixes # This is the commit message #162: fixes # This is the commit message #163: fixes # This is the commit message #164: fixes # This is the commit message #165: fixes # This is the commit message #166: fixes # This is the commit message #167: fixes # This is the commit message #168: fixes # This is the commit message #169: fixes # This is the commit message #170: fixes # This is the commit message #171: fixes # This is the commit message #172: fixes # This is the commit message #173: fixes # This is the commit message #174: fixes # This is the commit message #175: fixes # This is the commit message #176: fixes # This is the commit message #177: fixes # This is the commit message #178: fixes # This is the commit message #179: fixes # This is the commit message #180: fixes # This is the commit message #181: fixes # This is the commit message #182: fixes # This is the commit message #183: fixes # This is the commit message #184: fixes # This is the commit message #185: fixes # This is the commit message #186: fixes
#128
First commit. Lookup of Transactions that have already been added to a block.