Skip to content
This repository has been archived by the owner on Feb 12, 2025. It is now read-only.

Commit

Permalink
VG-1728: Remove the use of internal state in keychains (#831)
Browse files Browse the repository at this point in the history
* fix: CommonBitcoinLikeKeychains modifies outdated internal state

These changes make sure the internal state is loaded from pertistence before updating it.

* test: integ test on CommonBitcoinLikeKeychains to load state at init

* fix: CommonBitcoinLikeKeychains now loads state for every use

* test: integ test on CommonBitcoinLikeKeychains to fetch latest state before updating it
  • Loading branch information
jcoatelen-ledger authored Nov 29, 2021
1 parent 1065916 commit 528f2c0
Show file tree
Hide file tree
Showing 6 changed files with 243 additions and 59 deletions.
123 changes: 70 additions & 53 deletions core/src/wallet/bitcoin/keychains/CommonBitcoinLikeKeychains.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -72,58 +72,71 @@ namespace ledger {
.setNode(CHANGE).getPath();
_internalNodeXpub = std::static_pointer_cast<BitcoinLikeExtendedPublicKey>(_xpub)->derive(localPath);
}
_observableRange = (uint32_t) configuration->getInt(api::Configuration::KEYCHAIN_OBSERVABLE_RANGE)
.value_or(api::ConfigurationDefaults::KEYCHAIN_DEFAULT_OBSERVABLE_RANGE);
}

// Try to restore the state from preferences
auto state = preferences->getData("state", {});
if (!state.empty()) {
boost::iostreams::array_source my_vec_source(reinterpret_cast<char*>(&state[0]), state.size());
KeychainPersistentState CommonBitcoinLikeKeychains::getState() const {
// Try to get the state from preferences
auto preferences = getPreferences();
if(preferences == nullptr) {
throw make_exception(api::ErrorCode::NULL_POINTER, "Preferences is null.");
}
KeychainPersistentState state;
auto rawState = preferences->getData("state", {});
if (!rawState.empty()) {

boost::iostreams::array_source my_vec_source(reinterpret_cast<char*>(&rawState[0]), rawState.size());
boost::iostreams::stream<boost::iostreams::array_source> is(my_vec_source);
::cereal::BinaryInputArchive archive(is);
archive(_state);
} else {
_state.maxConsecutiveReceiveIndex = 0;
_state.maxConsecutiveChangeIndex = 0;
_state.empty = true;
}
_observableRange = (uint32_t) configuration->getInt(api::Configuration::KEYCHAIN_OBSERVABLE_RANGE)
.value_or(api::ConfigurationDefaults::KEYCHAIN_DEFAULT_OBSERVABLE_RANGE);
archive(state);
}
return state;
}

bool CommonBitcoinLikeKeychains::markPathAsUsed(const DerivationPath &p, bool needExtendKeychain) {
KeychainPersistentState state = getState();
DerivationPath path(p);

auto isToUpdate = [&path](const uint32_t& maxConsecutiveIndex, const std::set<uint32_t>& nonConsecutiveIndexes) {
return path.getLastChildNum() >= maxConsecutiveIndex && nonConsecutiveIndexes.find(path.getLastChildNum()) == nonConsecutiveIndexes.end();
};

auto updateState = [&path](uint32_t& maxConsecutiveIndex, std::set<uint32_t>& nonConsecutiveIndexes, bool& empty) {
if (path.getLastChildNum() == maxConsecutiveIndex)
maxConsecutiveIndex += 1;
else
nonConsecutiveIndexes.insert(path.getLastChildNum());
empty = false;
};


if (path.getParent().getLastChildNum() == 0) {
if (path.getLastChildNum() < _state.maxConsecutiveReceiveIndex ||
_state.nonConsecutiveReceiveIndexes.find(path.getLastChildNum()) != _state.nonConsecutiveReceiveIndexes.end()) {

if (!isToUpdate(state.maxConsecutiveReceiveIndex, state.nonConsecutiveReceiveIndexes)) {
return false;
} else {
if (path.getLastChildNum() == _state.maxConsecutiveReceiveIndex)
_state.maxConsecutiveReceiveIndex += 1;
else
_state.nonConsecutiveReceiveIndexes.insert(path.getLastChildNum());
_state.empty = false;
saveState();
if (needExtendKeychain) {
getAllObservableAddresses(path.getLastChildNum(), path.getLastChildNum() + _observableRange);
}
return true;
}

updateState(state.maxConsecutiveReceiveIndex, state.nonConsecutiveReceiveIndexes, state.empty);
saveState(std::move(state));
if (needExtendKeychain) {
getAllObservableAddresses(path.getLastChildNum(), path.getLastChildNum() + _observableRange);
}

} else {
if (path.getLastChildNum() < _state.maxConsecutiveChangeIndex ||
_state.nonConsecutiveChangeIndexes.find(path.getLastChildNum()) != _state.nonConsecutiveChangeIndexes.end()) {

if (!isToUpdate(state.maxConsecutiveChangeIndex, state.nonConsecutiveChangeIndexes)) {
return false;
} else {
if (path.getLastChildNum() == _state.maxConsecutiveChangeIndex)
_state.maxConsecutiveChangeIndex += 1;
else
_state.nonConsecutiveChangeIndexes.insert(path.getLastChildNum());
_state.empty = false;
saveState();
if (needExtendKeychain) {
getAllObservableAddresses(path.getLastChildNum(), path.getLastChildNum() + _observableRange);
}
return true;
}

updateState(state.maxConsecutiveChangeIndex, state.nonConsecutiveChangeIndexes, state.empty);
saveState(std::move(state));
if (needExtendKeychain) {
getAllObservableAddresses(path.getLastChildNum(), path.getLastChildNum() + _observableRange);
}
}

return true;
}

//TODO the two following methods are similar. Merge them if possible
Expand Down Expand Up @@ -200,16 +213,18 @@ namespace ledger {
}

BitcoinLikeKeychain::Address CommonBitcoinLikeKeychains::getFreshAddress(BitcoinLikeKeychain::KeyPurpose purpose) {
return derive(purpose, (purpose == KeyPurpose::RECEIVE ? _state.maxConsecutiveReceiveIndex : _state.maxConsecutiveChangeIndex));
KeychainPersistentState state = getState();
return derive(purpose, (purpose == KeyPurpose::RECEIVE ? state.maxConsecutiveReceiveIndex : state.maxConsecutiveChangeIndex));
}

bool CommonBitcoinLikeKeychains::isEmpty() const {
return _state.empty;
return getState().empty;
}

std::vector<BitcoinLikeKeychain::Address>
CommonBitcoinLikeKeychains::getFreshAddresses(BitcoinLikeKeychain::KeyPurpose purpose, size_t n) {
auto startOffset = (purpose == KeyPurpose::RECEIVE) ? _state.maxConsecutiveReceiveIndex : _state.maxConsecutiveChangeIndex;
KeychainPersistentState state = getState();
auto startOffset = (purpose == KeyPurpose::RECEIVE) ? state.maxConsecutiveReceiveIndex : state.maxConsecutiveChangeIndex;
std::vector<BitcoinLikeKeychain::Address> result(n);
for (auto i = 0; i < n; i++) {
result[i] = derive(purpose, startOffset + i);
Expand Down Expand Up @@ -242,7 +257,8 @@ namespace ledger {
std::vector<BitcoinLikeKeychain::Address>
CommonBitcoinLikeKeychains::getAllObservableAddresses(BitcoinLikeKeychain::KeyPurpose purpose, uint32_t from,
uint32_t to) {
auto maxObservableIndex = (purpose == KeyPurpose::CHANGE ? _state.maxConsecutiveChangeIndex + _state.nonConsecutiveChangeIndexes.size() : _state.maxConsecutiveReceiveIndex + _state.nonConsecutiveReceiveIndexes.size()) + _observableRange;
KeychainPersistentState state = getState();
auto maxObservableIndex = (purpose == KeyPurpose::CHANGE ? state.maxConsecutiveChangeIndex + state.nonConsecutiveChangeIndexes.size() : state.maxConsecutiveReceiveIndex + state.nonConsecutiveReceiveIndexes.size()) + _observableRange;
auto length = std::min<size_t >(to - from, maxObservableIndex - from);
std::vector<BitcoinLikeKeychain::Address> result;
result.reserve(length + 1);
Expand All @@ -256,18 +272,18 @@ namespace ledger {
return result;
}

void CommonBitcoinLikeKeychains::saveState() {
while (_state.nonConsecutiveReceiveIndexes.find(_state.maxConsecutiveReceiveIndex) != _state.nonConsecutiveReceiveIndexes.end()) {
_state.nonConsecutiveReceiveIndexes.erase(_state.maxConsecutiveReceiveIndex);
_state.maxConsecutiveReceiveIndex += 1;
void CommonBitcoinLikeKeychains::saveState(KeychainPersistentState state) const {
while (state.nonConsecutiveReceiveIndexes.find(state.maxConsecutiveReceiveIndex) != state.nonConsecutiveReceiveIndexes.end()) {
state.nonConsecutiveReceiveIndexes.erase(state.maxConsecutiveReceiveIndex);
state.maxConsecutiveReceiveIndex += 1;
}
while (_state.nonConsecutiveChangeIndexes.find(_state.maxConsecutiveChangeIndex) != _state.nonConsecutiveChangeIndexes.end()) {
_state.nonConsecutiveChangeIndexes.erase(_state.maxConsecutiveChangeIndex);
_state.maxConsecutiveChangeIndex += 1;
while (state.nonConsecutiveChangeIndexes.find(state.maxConsecutiveChangeIndex) != state.nonConsecutiveChangeIndexes.end()) {
state.nonConsecutiveChangeIndexes.erase(state.maxConsecutiveChangeIndex);
state.maxConsecutiveChangeIndex += 1;
}
std::stringstream is;
::cereal::BinaryOutputArchive archive(is);
archive(_state);
archive(state);
auto savedState = is.str();
getPreferences()->edit()->putData("state", std::vector<uint8_t>((const uint8_t *)savedState.data(),(const uint8_t *)savedState.data() + savedState.size()))->commit();
}
Expand All @@ -289,17 +305,18 @@ namespace ledger {
}

std::vector<BitcoinLikeKeychain::Address> CommonBitcoinLikeKeychains::getAllAddresses() {
KeychainPersistentState state = getState();
std::vector<BitcoinLikeKeychain::Address> addresses;
addresses.reserve(_state.maxConsecutiveChangeIndex + 1 + _state.maxConsecutiveReceiveIndex + 1);
addresses.reserve(state.maxConsecutiveChangeIndex + 1 + state.maxConsecutiveReceiveIndex + 1);

auto fetchAddressesFrom = [&](auto const keyPurpose, auto const maxIndex) {
for (auto i = 0; i <= maxIndex; ++i) {
addresses.push_back(derive(keyPurpose, i));
}
};

fetchAddressesFrom(KeyPurpose::CHANGE, _state.maxConsecutiveChangeIndex);
fetchAddressesFrom(KeyPurpose::RECEIVE, _state.maxConsecutiveReceiveIndex);
fetchAddressesFrom(KeyPurpose::CHANGE, state.maxConsecutiveChangeIndex);
fetchAddressesFrom(KeyPurpose::RECEIVE, state.maxConsecutiveReceiveIndex);

return addresses;
}
Expand Down
18 changes: 13 additions & 5 deletions core/src/wallet/bitcoin/keychains/CommonBitcoinLikeKeychains.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ namespace ledger {
namespace core {

struct KeychainPersistentState {
uint32_t maxConsecutiveChangeIndex;
uint32_t maxConsecutiveReceiveIndex;
uint32_t maxConsecutiveChangeIndex {0};
uint32_t maxConsecutiveReceiveIndex {0};
std::set<uint32_t> nonConsecutiveChangeIndexes;
std::set<uint32_t> nonConsecutiveReceiveIndexes;
bool empty;
bool empty {true};

template <class Archive>
void serialize(Archive& archive, std::uint32_t const version) {
Expand Down Expand Up @@ -87,6 +87,14 @@ namespace ledger {

Option<std::vector<uint8_t>> getPublicKey(const std::string &address) const override;


/**
* @brief Get the State object from user preferences
*
* @return persisted state
*/
KeychainPersistentState getState() const;

protected:
std::shared_ptr<api::BitcoinLikeExtendedPublicKey> _internalNodeXpub;
std::shared_ptr<api::BitcoinLikeExtendedPublicKey> _publicNodeXpub;
Expand All @@ -95,8 +103,8 @@ namespace ledger {

private:
BitcoinLikeKeychain::Address derive(KeyPurpose purpose, off_t index);
void saveState();
KeychainPersistentState _state;
void saveState(KeychainPersistentState state) const;

std::shared_ptr<api::BitcoinLikeExtendedPublicKey> _xpub;
};
}
Expand Down
2 changes: 1 addition & 1 deletion core/test/integration/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ else()
target_link_libraries(ledger-core-integration-tests ledger-core-static)
endif()


target_link_libraries(ledger-core-integration-tests gmock)
target_link_libraries(ledger-core-integration-tests ledger-test)
target_include_directories(ledger-core-integration-tests PUBLIC ../../../core/src)

Expand Down
154 changes: 154 additions & 0 deletions core/test/integration/keychains/common_bitcoin_keychain_test.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
/*
*
* p2sh_keychain_test
* ledger-core
*
* Created by El Khalil Bellakrid on 01/05/2018.
*
* The MIT License (MIT)
*
* Copyright (c) 2018 Ledger
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in all
* copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
* SOFTWARE.
*
*/

#include <gtest/gtest.h>
#include <gmock/gmock.h>
#include "api/PreferencesChange.hpp"
#include "api/KeychainEngines.hpp"
#include "keychain_test_helper.h"

using ::testing::_;
using ::testing::Return;
using ::testing::Invoke;

class MockPreferencesBackend: public api::PreferencesBackend {
public:
MOCK_METHOD(std::experimental::optional<std::vector<uint8_t>>, get, (const std::vector<uint8_t> &));
MOCK_METHOD(bool, commit, (const std::vector<api::PreferencesChange> &));
MOCK_METHOD(void, setEncryption, (const std::shared_ptr<api::RandomNumberGenerator> &, const std::string &));
MOCK_METHOD(void, unsetEncryption, ());
MOCK_METHOD(bool, resetEncryption, (const std::shared_ptr<api::RandomNumberGenerator> &, const std::string &, const std::string &));
MOCK_METHOD(std::string, getEncryptionSalt, ());
MOCK_METHOD(void, clear, ());

};


class ConcreteCommonBitcoinLikeKeychains: public CommonBitcoinLikeKeychains {
public:
ConcreteCommonBitcoinLikeKeychains(
const std::shared_ptr<api::DynamicObject> &configuration,
const api::Currency &params,
int account,
const std::shared_ptr<api::BitcoinLikeExtendedPublicKey> &xpub,
const std::shared_ptr<Preferences> &preferences)
: CommonBitcoinLikeKeychains(configuration, params, account, xpub, preferences)
{
_keychainEngine = api::KeychainEngines::BIP49_P2SH;
}
int32_t getOutputSizeAsSignedTxInput() const override { return 0; }
};

class CommonBitcoinKeychains : public KeychainFixture<ConcreteCommonBitcoinLikeKeychains> {
};

template <typename T>
std::vector<T> string2vector(const std::string& s) {
return std::vector<T>(s.begin(), s.end());
}

std::string serializeStateToString(const KeychainPersistentState& state) {
std::stringstream is;
::cereal::BinaryOutputArchive archive(is);
archive(state);
return is.str();
}

std::vector<uint8_t> serializeStateToVector(const KeychainPersistentState& state) {
const std::string savedState = serializeStateToString(state);
return std::vector<uint8_t>((const uint8_t *)savedState.data(),(const uint8_t *)savedState.data() + savedState.size());
}

TEST_F(CommonBitcoinKeychains, CorrectStateAtInitialization) {

KeychainPersistentState mockState;
mockState.maxConsecutiveChangeIndex = 4;
mockState.nonConsecutiveChangeIndexes.emplace(1);
mockState.nonConsecutiveChangeIndexes.emplace(2);
mockState.nonConsecutiveChangeIndexes.emplace(3);
mockState.maxConsecutiveReceiveIndex = 4;
mockState.nonConsecutiveReceiveIndexes.emplace(1);
mockState.nonConsecutiveReceiveIndexes.emplace(2);
mockState.nonConsecutiveReceiveIndexes.emplace(3);

auto backend = std::make_shared<MockPreferencesBackend>();
EXPECT_CALL(*backend, get(string2vector<uint8_t>("keychainstate")))
.Times(1)
.WillOnce(Return(serializeStateToVector(mockState)));

testKeychain(BTC_TESTNET_DATA, backend, [&backend, &mockState] (CommonBitcoinLikeKeychains& keychain) {
const KeychainPersistentState& state = keychain.getState();
EXPECT_EQ(mockState.maxConsecutiveChangeIndex, state.maxConsecutiveChangeIndex);
EXPECT_EQ(mockState.maxConsecutiveReceiveIndex, state.maxConsecutiveReceiveIndex);
EXPECT_EQ(mockState.nonConsecutiveChangeIndexes.size(), state.nonConsecutiveChangeIndexes.size());
EXPECT_EQ(mockState.nonConsecutiveReceiveIndexes.size(), state.nonConsecutiveReceiveIndexes.size());
});
}


TEST_F(CommonBitcoinKeychains, CorrectStateProducedByMarkPathAsUsed) {

testKeychain(BTC_TESTNET_DATA, [] (CommonBitcoinLikeKeychains& keychain) {

auto addresses = keychain.getAllObservableAddresses(0, 10);
EXPECT_TRUE(keychain.markAsUsed("2N3uTrmyNePhAbiUxi8uq7P2J7SxS2bCaji"));

const KeychainPersistentState& state = keychain.getState();
EXPECT_EQ(0, state.maxConsecutiveChangeIndex);
EXPECT_EQ(0, state.maxConsecutiveReceiveIndex);
EXPECT_EQ(1, state.nonConsecutiveChangeIndexes.size());
EXPECT_EQ(0, state.nonConsecutiveReceiveIndexes.size());
});
}


TEST_F(CommonBitcoinKeychains, CorrectStateUsedAtMarkPathAsUsed) {

testKeychain(BTC_TESTNET_DATA, [] (ConcreteCommonBitcoinLikeKeychains& keychain) {

auto addresses = keychain.getAllObservableAddresses(0, 10);

// update state from outside
KeychainPersistentState stateInput = keychain.getState();
stateInput.maxConsecutiveChangeIndex = 0;
stateInput.nonConsecutiveChangeIndexes.insert(0);
keychain.getPreferences()->edit()->putData("state", serializeStateToVector(stateInput))->commit();

EXPECT_TRUE(keychain.markAsUsed("2N3uTrmyNePhAbiUxi8uq7P2J7SxS2bCaji"));

const KeychainPersistentState& state = keychain.getState();
EXPECT_EQ(2, state.maxConsecutiveChangeIndex);
EXPECT_EQ(0, state.maxConsecutiveReceiveIndex);
EXPECT_EQ(0, state.nonConsecutiveChangeIndexes.size());
EXPECT_EQ(0, state.nonConsecutiveReceiveIndexes.size());
});
}

Loading

0 comments on commit 528f2c0

Please sign in to comment.