Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(world_state): fix race conditions in WorldState and IndexedTree #8612

Merged
merged 4 commits into from
Sep 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions barretenberg/cpp/CMakePresets.json
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,9 @@
"description": "Build with thread sanitizer on clang16 with debugging information",
"inherits": "clang16-dbg",
"binaryDir": "build-tsan",
"cacheVariables": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Needed to compile with tsan

"HAVE_STD_REGEX": "ON"
},
"environment": {
"CFLAGS": "-fsanitize=thread",
"CXXFLAGS": "-fsanitize=thread",
Expand Down
31 changes: 15 additions & 16 deletions barretenberg/cpp/src/barretenberg/crypto/merkle_tree/signal.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,27 @@

namespace bb::crypto::merkle_tree {
/**
* @brief Used in parallel insertions in the IndexedTree. Workers signal to other following workes as they move up
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a revert of misguided changes I made

* @brief Used in parallel insertions in the the IndexedTree. Workers signal to other following workes as they move up
* the level of the tree.
*
*/
class Signal {
public:
explicit Signal(uint32_t initial_level = 1)
: signal_(std::make_unique<std::atomic<uint32_t>>(initial_level)){};
Signal(uint32_t initial_level = 1)
: signal_(initial_level){};
~Signal() = default;
Signal(const Signal& other)
: signal_(std::make_unique<std::atomic<uint32_t>>(other.signal_->load()))
: signal_(other.signal_.load())
{}
Signal(Signal&& other) = default;
Signal(const Signal&& other) = delete;
Signal& operator=(const Signal& other)
{
if (this != &other) {
signal_->store(other.signal_->load());
signal_.store(other.signal_.load());
}
return *this;
}
Signal& operator=(Signal&& other) = default;
Signal& operator=(const Signal&& other) = delete;

/**
* @brief Causes the thread to wait until the required level has been signalled
Expand All @@ -33,10 +33,10 @@ class Signal {
*/
void wait_for_level(uint32_t level = 0)
{
uint32_t current_level = signal_->load();
uint32_t current_level = signal_.load();
while (current_level > level) {
signal_->wait(current_level);
current_level = signal_->load();
signal_.wait(current_level);
current_level = signal_.load();
}
}

Expand All @@ -47,18 +47,17 @@ class Signal {
*/
void signal_level(uint32_t level = 0)
{
signal_->store(level);
signal_->notify_all();
signal_.store(level);
signal_.notify_all();
}

void signal_decrement(uint32_t delta = 1)
{
signal_->fetch_sub(delta);
signal_->notify_all();
signal_.fetch_sub(delta);
signal_.notify_all();
}

private:
// apple clang has issues if this cannot be move-constructed, so we wrap in unique_ptr
std::unique_ptr<std::atomic<uint32_t>> signal_;
std::atomic<uint32_t> signal_;
};
} // namespace bb::crypto::merkle_tree
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ template <class DeciderProvingKeys_> class ProtogalaxyProverInternal {
* time) assumes the value G(1) is 0, which is true in the case where the witness to be folded is valid.
* @todo (https://github.com/AztecProtocol/barretenberg/issues/968) Make combiner tests better
*
* @tparam skip_zero_computations whether to use the the optimization that skips computing zero.
* @tparam skip_zero_computations whether to use the optimization that skips computing zero.
* @param
* @param gate_separators
* @return ExtendedUnivariateWithRandomization
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,11 @@ StateReference WorldState::get_state_reference(WorldStateRevision revision) cons
{
Signal signal(static_cast<uint32_t>(_trees.size()));
StateReference state_reference;
// multiple threads want to write to state_reference
std::mutex state_ref_mutex;

bool uncommitted = include_uncommitted(revision);

for (const auto& [id, tree] : _trees) {
auto callback = [&signal, &state_reference, &state_ref_mutex, id](const TypedResponse<TreeMetaResponse>& meta) {
auto callback = [&](const TypedResponse<TreeMetaResponse>& meta) {
std::lock_guard<std::mutex> lock(state_ref_mutex);
state_reference.insert({ id, { meta.inner.root, meta.inner.size } });
signal.signal_decrement();
Expand Down
2 changes: 2 additions & 0 deletions barretenberg/cpp/src/barretenberg/world_state/world_state.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ class WorldState {
std::unique_ptr<crypto::merkle_tree::LMDBEnvironment> _lmdb_env;
std::unordered_map<MerkleTreeId, Tree> _trees;
bb::ThreadPool _workers;
// Guards state reference access, flagged as mutable as used in otherwise const methods
mutable std::mutex state_ref_mutex;

TreeStateReference get_tree_snapshot(MerkleTreeId id);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ class WorldStateTest : public testing::Test {
protected:
void SetUp() override
{
GTEST_SKIP();
// setup with 1MB max db size, 1 max database and 2 maximum concurrent readers
_directory = random_temp_directory();
std::filesystem::create_directories(_directory);
Expand Down
2 changes: 1 addition & 1 deletion docs/docs/tutorials/codealong/cli_wallet/faceid_wallet.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Aztec is in active development and this has only been tested on MacOS. Please re

## Prerequisites

For this tutorial, we will need to have the the [Sandbox](../../../reference/developer_references/sandbox_reference/index.md) installed.
For this tutorial, we will need to have the [Sandbox](../../../reference/developer_references/sandbox_reference/index.md) installed.

We also need to install Secretive, a nice open-source package that allows us to store keys on the Secure Enclave. You can head to the [secretive releases page](https://github.com/maxgoedjen/secretive/releases) and get the last release's `zip`, unzip and move to Applications, or use [Homebrew](https://brew.sh/):

Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/src/core/interfaces/messagebridge/IOutbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ interface IOutbox {
* @param _l2BlockNumber - The block number to fetch the root data for
*
* @return root - The root of the merkle tree containing the L2 to L1 messages
* @return minHeight - The min height for the the merkle tree that the root corresponds to
* @return minHeight - The min height for the merkle tree that the root corresponds to
*/
function getRootData(uint256 _l2BlockNumber)
external
Expand Down
2 changes: 1 addition & 1 deletion l1-contracts/src/core/messagebridge/Outbox.sol
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ contract Outbox is IOutbox {
* @param _l2BlockNumber - The block number to fetch the root data for
*
* @return root - The root of the merkle tree containing the L2 to L1 messages
* @return minHeight - The min height for the the merkle tree that the root corresponds to
* @return minHeight - The min height for the merkle tree that the root corresponds to
*/
function getRootData(uint256 _l2BlockNumber)
external
Expand Down
Loading