Merge blockvault into develop #9705
Conversation
Merge branch 'blockvault' of github.com:EOSIO/eos into blockvault
mock web server&client plugin with beta interface
This reverts commit 5050336.
This reverts commit 70117af.
This reverts commit 8a1abaa.
This reverts commit 785d404.
This reverts commit bc5599d.
Revert `blockvault` to stable state
…ckvault Merge `develop` into `blockvault`
| @@ -46,7 +46,18 @@ RUN curl -LO https://dl.bintray.com/boostorg/release/1.72.0/source/boost_1_72_0. | |||
| ./bootstrap.sh --with-toolset=clang --prefix=/usr/local && \ | |||
| ./b2 toolset=clang cxxflags='-stdlib=libc++ -D__STRICT_ANSI__ -nostdinc++ -I/usr/local/include/c++/v1 -D_FORTIFY_SOURCE=2 -fstack-protector-strong -fpie' linkflags='-stdlib=libc++ -pie' link=static threading=multi --with-iostreams --with-date_time --with-filesystem --with-system --with-program_options --with-chrono --with-test -q -j$(nproc) install && \ | |||
| cd / && \ | |||
| rm -rf boost_1_72_0.tar.bz2 /boost_1_72_0 | |||
| rm -rf boost_1_72_0.tar.bz2 /boost_1_72_0whi | |||
There was a problem hiding this comment.
yes, wIll be fixed in PR #9711
|
@huangminghuang I've completed a logic review and turned it to approve. When you and @johndebord have the build/ci issues resolved it can go in as far as I am concerned. |
tests/CMakeLists.txt
Outdated
| @@ -105,6 +106,7 @@ add_test(NAME keosd_auto_launch_test COMMAND tests/keosd_auto_launch_test.py WOR | |||
| set_property(TEST keosd_auto_launch_test PROPERTY LABELS nonparallelizable_tests) | |||
| add_test(NAME db_modes_test COMMAND tests/db_modes_test.sh WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) | |||
| set_tests_properties(db_modes_test PROPERTIES COST 6000) | |||
| set_property(TEST db_modes_test PROPERTY LABELS nonparallelizable_tests) | |||
There was a problem hiding this comment.
Why is this needed? That test is designed to be parallelizable.
There was a problem hiding this comment.
It listens on some default TCP ports which cannot be shared with others.
There was a problem hiding this comment.
What ports? It's launched with --http-server-address '' --p2p-listen-endpoint '' which has historically disabled listening on all ports.
There was a problem hiding this comment.
revert the change in commit b068313
| @@ -3,6 +3,8 @@ set -eo pipefail | |||
| VERSION=1 | |||
| brew update | |||
| brew install git cmake python libtool libusb graphviz automake wget gmp pkgconfig doxygen [email protected] jq || : | |||
| curl -LO https://raw.githubusercontent.com/Homebrew/homebrew-core/106b4b8a421dda33c79a4018c3c3816234076331/Formula/libpqxx.rb | |||
There was a problem hiding this comment.
We've done stuff like this before and were burned by it. Why are we doing these sort of hacks instead of just using it as a submodule like it appears to be designed for? @larryk85 why are you against this being a submodule?
There was a problem hiding this comment.
I'm not against submodules as a whole. I'm against this particular dependency being a submodule for a few reasons.
We need clearly defined rules as to what is or is not a submodule in our codebase. It is clear with some of the other recent additions as submodules that without solid rules this can lead to less than ideal situations (we have to install components or the entirety of the dependencies with our own installation, possibility of building certain things inefficiently for certain targets, etc).
This is also in effect to aid in consistency with our dependency management and overall project management. There are no questions as to what to do when or why, if we modify it or own it then it is a submodule. Other than that, it lives outside of that space and we have to treat it like any other external dependency. Also if that means that we need to have custom brew/repo mirrors for our needed versions to fill in certain targets, then that is a sufficient answer as well. Once again we don't modify these, as such the need to update them is going to be low.
We don't own libpqxx and we do not modify libpqxx, therefore it makes little sense at this point to have it in our codebase because it is probably the easier thing to do. I fully understand your points and if in the future we need a modified variant of the lib, then it can live in our codebase and install 'along-side' our main code install.
There was a problem hiding this comment.
We don't own rocksdb, we don't own yubihsm, we don't own rapidjson, we don't own libsecp256k1, and we don't own amqp-cpp either. Nor did we modify those. Not submoduling looks contrarian not consistent.
It's not just the "easier thing to do", being a submodule has many obvious engineering benefits to us and usability benefits to users:
- Can universally, consistently pin at a known good version; something this PR does multiple ways contrary to your "aid in consistency with our dependency management" goal
- Seamlessly works for pinned builds in all environments; something this PR doesn't handle in all cases AFAIK
- Easier to install the required license file; something this PR doesn't handle
- Doesn't require users to chase down a dependency that isn't provided by common distros (we should not start leaning on the build scripts to do that -- those need to go away. If we need a build script we've made it too hard. Good OSS should not require some obfuscated build script)
Instead we're spending significant engineering time crafting fragile unique solutions to these issues scattered throughout the CI and build scripts in an inconsistent way.
| @@ -1131,6 +1143,10 @@ void producer_plugin::create_snapshot(producer_plugin::next_function<producer_pl | |||
| ("message", ec.message())); | |||
|
|
|||
| my->_pending_snapshot_index.emplace(head_id, next, pending_path.generic_string(), snapshot_path.generic_string()); | |||
|
|
|||
| if ( my->blockvault != nullptr ) { | |||
| my->blockvault->propose_snapshot( blockvault::watermark_t{head_block_num, head_block_time}, pending_path.generic_string().c_str() ); | |||
There was a problem hiding this comment.
What if this block gets forked out?
There was a problem hiding this comment.
Does look like this should be called from pending_snapshot::finalize
addressed in commit 020c286 |
Change Description
This PR merges the blockvault branch into develop
Change Type
Select ONE:
Testing Changes
Select ANY that apply:
Consensus Changes
API Changes
Documentation Additions
plugins/blockvault_client_plugin/README.md provides some basic documentation for the overview and configurations.