Update SHiP to work with RocksDB #9635

victorj8 · 2020-11-03T21:01:48Z

Change Description

To update SHiP to work with RocksDB the following approach was followed:

2 methods were added to the current combined_database class to access its chainbase and rocksdb members.
The method create_deltas was modified to take a combined_database instead of only a chainbase object.
The current method that creates the deltas from the chainbase object was left intact.
To generate the deltas for the information stored on rocksdb, some common code on the rocksdb efforts were rearranged to walk through this data and be able to gather the different information we are interested (for contracts and kv).
After we obtain this data, new serialization methods were also required to be able to serialize this information into the deltas.
Once the deltas for the data on rocksdb are generated, they are appended to the ones generated on chainbase.
The SHiP unit tests for delta generation were updated to be tested with chainbase and rocksdb as backing store.
Some modification to these unit tests was also done, specially to test the deletion of objects, for that reason a smart contract used for testing was also updated.

Change Type

Select ONE

Documentation

Stability bug fix

Other

Other - special case

Consensus Changes

Consensus Changes

API Changes

API Changes

Documentation Additions

Documentation Additions

… create_deltas.

…o database.

…efore end key reached.

swatanabe-b1

Hit 'n run

libraries/state_history/create_deltas.cpp

swatanabe-b1 · 2020-11-04T20:10:12Z

libraries/chain/include/eosio/chain/backing_store/db_combined.hpp

+   if (!key) {
+      return;
+   }
+   const auto end_key = key.next_sub_key();


should be next.

swatanabe-b1 · 2020-11-04T20:10:27Z

libraries/chain/include/eosio/chain/backing_store/db_combined.hpp

+// will walk through all entries with the given prefix, so if passed an exact key, it will match that key
+// and any keys with that key as a prefix
+template <typename Receiver, typename Function = std::decay_t < decltype(process_all)>>
+void walk_any_rocksdb_entries_with_prefix(const kv_undo_stack_ptr& kv_undo_stack,


Is this function used anywhere?

no, we can remove it

swatanabe-b1 · 2020-11-04T20:17:03Z

libraries/chain_kv/include/b1/session/shared_bytes.hpp

+   return eosio::session::shared_bytes(buffer.data(), buffer.size());
+}
+
+inline shared_bytes shared_bytes::previous() const {


Is this used?

He and I were working off the same branch. My code needs it, but since I changed it since then, it would be fine to remove and then let me deal with the merge problems.

swatanabe-b1 · 2020-11-04T20:22:18Z

libraries/chain_kv/include/b1/session/shared_bytes.hpp

+   int index = buffer.size() - 1;
+   while (true) {
+      auto& val = buffer[index];
+      if (val) {
+         --val;
+         ++index; // ensure we increment any indexes past this one
+         break;
+      }
+      if (--index < 0) {
+         buffer.pop_back();
+         index = buffer.size();
+      }
+   }
+   while (index < buffer.size()) {
+      buffer[index] = std::numeric_limits<unsigned char>::max();
+   }


What is the intended behavior of previous? The behavior of x.next() can be defined as the lowest value y such x < y and x is not a prefix of y. The converse of this is that x.previous() is the greatest value y such that y < x (which implies that x is not a prefix of y). This algorithm does not fulfill this definition, and in fact, no such value exists.

I had this wrong. The logic of next is really incorrect I believe. It should just increment at the index, and then rollback to 0, and then increment the next lower index. In the corner case of 0xfff..., that is less straight forward. I am thinking that in that case we would want it to then just append 0xff at the end. The new previous() logic is to decrement each index (starting at the last), if it is greater than 0 then exit, otherwise it rolls back to 0xff and then goes to the next lower index and repeats. For 0x0000...00, it just shortens the vector by 1

The implementation of next is correct according to the definition I gave above (with the exception of FF* for which the "correct" result is undefined) and this matches how it is actually used. Based on how previous is used, (in chain_plugin.cpp read_only::get_table_by_scope), I don't think there is a way to define previous that will work correctly.

swatanabe-b1 · 2020-11-04T20:23:31Z

libraries/chain_kv/include/b1/session/shared_bytes.hpp

+
+   auto buffer = std::vector<unsigned char>{ std::begin(*this), std::end(*this) };
+
+   int index = buffer.size() - 1;


This limits the buffer to 2GiB.

swatanabe-b1 · 2020-11-04T20:26:40Z

libraries/chain_kv/include/b1/session/shared_bytes.hpp

+      }
+      if (--index < 0) {
+         buffer.pop_back();
+         index = buffer.size();


Needs to break, otherwise UB at the top of the next loop iteration.

This method has been changed quite a bit in my code.

swatanabe-b1 · 2020-11-04T20:46:50Z

libraries/chain/include/eosio/chain/backing_store/db_combined.hpp

+};
+
+template <typename F>
+void walk_rocksdb_entries_with_prefix(const kv_undo_stack_ptr& kv_undo_stack,


with_prefix is no longer accurate.

Will fix this in my PR to avoid having to deal with the conflicts

… get when creating the deltas

TimothyBanks · 2020-11-06T18:46:07Z

libraries/chain/combined_database.cpp

-      }
+   void add_kv_table_to_snapshot(const snapshot_writer_ptr& snapshot, const chainbase::database& db, const kv_undo_stack_ptr& kv_undo_stack) {
+      snapshot->write_section<kv_object>([&db,&kv_undo_stack](auto& section) {
+         if (kv_undo_stack && db.get<kv_db_config_object>().backing_store == backing_store_type::ROCKSDB) {


I think one implies the other here so we should only have to check the backing_store.

I agree, but would like to do that in my PR, since this check occurs in multiple places besides this one.

libraries/chain/combined_database.cpp

TimothyBanks · 2020-11-06T18:52:49Z

libraries/chain/combined_database.cpp

+            }
+         });
+         // write out any remaining key-values
+         kv_database->write(key_values);


Might want to check if there are actually any values to write. This method won't entirely be a no-op if the container is empty.

That seems like a good optimization to put into that write method. Can you do it as part of your PR for the replay fixes?

libraries/chain/combined_database.cpp

TimothyBanks · 2020-11-06T19:18:00Z

libraries/chain/include/eosio/chain/backing_store/db_combined.hpp

+   virtual void wrong_row(const char* type_received) = 0;
+};
+namespace detail {
+   class manage_stack {


Something to make a note of, with the recent changes to fix replay (https://github.com/EOSIO/eos/pull/9642/files) this will no longer be needed. The undo_stack will now always return something when top is called.

libraries/state_history/include/eosio/state_history/rocksdb_receiver.hpp

libraries/rodeos/include/b1/rodeos/rodeos_tables.hpp

libraries/state_history/include/eosio/state_history/rocksdb_receiver.hpp

libraries/rodeos/include/b1/rodeos/rodeos_tables.hpp

libraries/state_history/include/eosio/state_history/rocksdb_receiver.hpp

…e unit test cases

brianjohnson5972

I'd like the #warning added, but otherwise it is approved.

victorj8 and others added 29 commits October 22, 2020 11:04

move some useful common code from combined_database.cpp

1c78b5f

ship - rocksdb (wip)

8f0ae9b

Merge remote-tracking branch 'origin/develop' into rocksdb_ship

1badb85

ship - rocksdb integrationg (wip)

de4fe84

Merge branch 'develop' into rocksdb_ship

f65fa4f

modify key_value_v0 to erase database field and add payer

763767e

update abieos

42eadad

some naming changes

47edcd4

Refactor kv snapshots to use a writer that will be available for ship…

1c3e2a7

… create_deltas.

Improved kv snapshots test to have multiple contracts' data written t…

54c8f83

…o database.

Moved KV/DB prefix flag to db_combined and made them both a single char.

94216bd

Added next_sub_key to return the very next key.

c8e1485

Added lambda to rocksdb_contract_kv_table_writer to stop processing b…

5c8b84e

…efore end key reached.

Refactored db writer to separate table collection logic.

f143313

Fixed rebase errors.

54e10d3

More rebase fixes.

7a798b2

Another rebase error.

ad5fe59

Correctly wrapping kv_receiver.

8c0e579

fix problem with chainbase - rocksdb tester setup

0fb929a

Added method to process an individual rocksdb entry.

3a423c4

ship - rocksdb (wip)

ae9f577

rocksdb - ship (wip) all current unit tests passing

0832136

ship - rocksdb refactoring rocksdb receiver and unit test updates

8448936

update of state history delta tests with rocksdb and update a comment

c7fb44f

Merge branch 'develop' into rocksdb_ship

88fa62e

change #warning to TODO

8604b47

fix db_previous_i64 method

bfbd460

add unit tests with both kv and multi-index

87c1e8e

Merge branch 'develop' into rocksdb_ship

017795e

victorj8 requested a review from tbfleming November 3, 2020 21:01

victorj8 requested review from larryk85 and brianjohnson5972 November 3, 2020 21:01

swatanabe-b1 reviewed Nov 5, 2020

View reviewed changes

victorj8 added 2 commits November 5, 2020 11:20

fix extra comma on json

971dc82

make section of test_deltas_contract test independent of the order we…

13ab1b4

… get when creating the deltas

TimothyBanks reviewed Nov 6, 2020

View reviewed changes

brianjohnson5972 suggested changes Nov 6, 2020

View reviewed changes

brianjohnson5972 reviewed Nov 9, 2020

View reviewed changes

libraries/state_history/include/eosio/state_history/rocksdb_receiver.hpp Outdated Show resolved Hide resolved

victorj8 added 3 commits November 9, 2020 14:57

Merge branch 'develop' into rocksdb_ship

afb2900

address comments on PR, refactor rocksdb receiver code and add/improv…

a4f0e13

…e unit test cases

address comments on the PR

8a46c4f

brianjohnson5972 approved these changes Nov 11, 2020

View reviewed changes

Changed todo comment into a #warning.

c8cf8d8

brianjohnson5972 merged commit 2077111 into develop Nov 11, 2020

swatanabe-b1 mentioned this pull request Nov 19, 2020

Rocksdb rpc support #9685

Merged

7 tasks

brianjohnson5972 deleted the rocksdb_ship branch November 21, 2020 15:04

heifner mentioned this pull request Jul 8, 2022

Backport SHiP tests eosnetworkfoundation/mandel#625

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update SHiP to work with RocksDB #9635

Update SHiP to work with RocksDB #9635

victorj8 commented Nov 3, 2020

swatanabe-b1 left a comment

swatanabe-b1 Nov 4, 2020

swatanabe-b1 Nov 4, 2020

brianjohnson5972 Nov 6, 2020

swatanabe-b1 Nov 4, 2020

brianjohnson5972 Nov 6, 2020

swatanabe-b1 Nov 4, 2020

brianjohnson5972 Nov 6, 2020

swatanabe-b1 Nov 6, 2020

swatanabe-b1 Nov 4, 2020

swatanabe-b1 Nov 4, 2020

brianjohnson5972 Nov 6, 2020

swatanabe-b1 Nov 4, 2020

brianjohnson5972 Nov 11, 2020

TimothyBanks Nov 6, 2020

brianjohnson5972 Nov 11, 2020

TimothyBanks Nov 6, 2020

brianjohnson5972 Nov 6, 2020 •

edited

Loading

TimothyBanks Nov 6, 2020

brianjohnson5972 left a comment


		auto buffer = std::vector<unsigned char>{ std::begin(this), std::end(this) };

		int index = buffer.size() - 1;

Update SHiP to work with RocksDB #9635

Update SHiP to work with RocksDB #9635

Conversation

victorj8 commented Nov 3, 2020

Change Description

Change Type

Consensus Changes

API Changes

Documentation Additions

swatanabe-b1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianjohnson5972 Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianjohnson5972 left a comment

Choose a reason for hiding this comment

brianjohnson5972 Nov 6, 2020 •

edited

Loading