Skip to content

Commit

Permalink
fix(world_state): fix race conditions in WorldState and IndexedTree (#…
Browse files Browse the repository at this point in the history
…8612)

One caused by me doing a 'mac build' fix, oops. Mac build temporarily
rebroken as no one relies on world_state on mac atm
- reverts bad Signal change, reverts to raw std::atomic
- puts std::mutex as a mutable class member as it guards accesses
- reenables the tests
- minor side-effect: fixes 'the the' usages
  • Loading branch information
ludamad authored Sep 18, 2024
1 parent 25d5805 commit 6797525
Show file tree
Hide file tree
Showing 9 changed files with 25 additions and 24 deletions.
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": {
"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
* @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

0 comments on commit 6797525

Please sign in to comment.