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

Initial Transaction API #130

Merged
merged 9 commits into from
Aug 8, 2017
Merged

Conversation

brianjohnson5972
Copy link
Contributor

#128
First commit. Lookup of Transactions that have already been added to a block.

@nathanielhourt
Copy link
Contributor

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?

@brianjohnson5972
Copy link
Contributor Author

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.

@brianjohnson5972
Copy link
Contributor Author

(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.

@bytemaster
Copy link
Contributor

Yes, please implement with chainbase.

@brianjohnson5972
Copy link
Contributor Author

@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.

@bytemaster
Copy link
Contributor

@brianjohnson5972 there were conflicts between this and Phil's work, can you please resolve them so that I can merge?

Copy link
Contributor

@nathanielhourt nathanielhourt left a 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;
Copy link
Contributor

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,
Copy link
Contributor

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
Copy link
Contributor

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"

Copy link
Contributor

@nathanielhourt nathanielhourt left a 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 ) );
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor Author

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( [&]() {
Copy link
Contributor

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?

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 believe I "fixed" that (Otherwise Travis would complain). see commit d48ebab.

Copy link
Contributor Author

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.

Copy link
Contributor

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 :]

@nathanielhourt nathanielhourt merged commit 4a18bde into EOSIO:master Aug 8, 2017
@0x83043
Copy link

0x83043 commented Aug 25, 2017

Hello, I caught the same error:
`
[ 97%] Building CXX object plugins/producer_plugin/CMakeFiles/producer_plugin.dir/producer_plugin.cpp.o
/root/eos/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( & {

/root/eos/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)() )
           ^
/root/eos/plugins/account_history_plugin/account_history_plugin.cpp:71:2: warning: TODO: lookup of recent transactions [-W#warnings]
#warning TODO: lookup of recent transactions
^
1 warning and 1 error generated.
plugins/account_history_plugin/CMakeFiles/account_history_plugin.dir/build.make:62: recipe for target 'plugins/account_history_plugin/CMakeFiles/account_history_plugin.dir/account_history_plugin.cpp.o' failed
make[2]: *** [plugins/account_history_plugin/CMakeFiles/account_history_plugin.dir/account_history_plugin.cpp.o] Error 1
CMakeFiles/Makefile2:3396: recipe for target 'plugins/account_history_plugin/CMakeFiles/account_history_plugin.dir/all' failed
make[1]: *** [plugins/account_history_plugin/CMakeFiles/account_history_plugin.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 97%] Linking CXX static library libproducer_plugin.a
[ 97%] Built target producer_plugin
[ 98%] Linking CXX static library libchain_api_plugin.a
[ 98%] Built target chain_api_plugin
Makefile:129: recipe for target 'all' failed
make: *** [all] Error 2
`
How you fixed it?

@brianjohnson5972
Copy link
Contributor Author

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.

@0x83043
Copy link

0x83043 commented Aug 25, 2017

It's didn't update with "git submodule update". But the same result (error) after force update

root@4c5c874888cf:~/eos# git submodule update
root@4c5c874888cf:~/eos# git submodule update -f
Submodule path 'libraries/appbase': checked out '754b9d19783e008aa783a12465594f1fe2a49c29'
Submodule path 'libraries/binaryen': checked out '3f0db5a7aafaaa4de713ff3ba3c3bbeb59fe566e'
Submodule path 'libraries/chainbase': checked out 'f1a150f97bfde1b10113b77c22206e5c00158205'

@brianjohnson5972
Copy link
Contributor Author

Sorry, you are correct, the submodule got changed to an earlier revision. We will get that fixed.

@0x83043
Copy link

0x83043 commented Aug 25, 2017

Ok. Please let me know, when I can try build it one more time.

@brianjohnson5972
Copy link
Contributor Author

What is on master now should build.

@0x83043
Copy link

0x83043 commented Aug 25, 2017

It's ok. Thanx )

ljrprocc pushed a commit to bithacks-tech/myeosio that referenced this pull request Jul 4, 2018
NorseGaud pushed a commit that referenced this pull request Jul 30, 2019
# 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
pnx pushed a commit to eosswedenorg/eos that referenced this pull request Nov 11, 2019
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.

4 participants