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

Initial libmw code review #703

Closed
wants to merge 3 commits into from
Closed

Conversation

DavidBurkett
Copy link
Member

@DavidBurkett DavidBurkett commented Mar 16, 2021

libmw is a completely self-contained project with clearly defined set of APIs that all calls from the existing Litecoin project go through.

It currently uses cmake and builds as a shared library, but to maintain consistency with the existing build & release process, we will eventually turn this into a makefile project instead, and build it as a static library.

I will create some docs later this week that will offer suggestions for how to go about reviewing the code, and will include API usage examples that you can use as a guide to understanding how libmw is used.

@tromp
Copy link

@tromp tromp commented Mar 16, 2021

What is SHA1 (/src/libmw/deps/litecoin/crypto/sha1.cpp) needed for?

@DavidBurkett
Copy link
Member Author

@DavidBurkett DavidBurkett commented Mar 16, 2021

What is SHA1 (/src/libmw/deps/litecoin/crypto/sha1.cpp) needed for?

I pulled in the whole crypto dir from the litecoin repo, and then started removing what was no longer needed. It looks like I missed quite a few of them though. We're not using siphash, scrypt, or sha1. I'll remove those.

Once we switch to making this a static library included as part of the parent makefile project, we should be able to get rid of the entire deps/litecoin directory (https://github.com/DavidBurkett/litecoin/tree/0.18/src/libmw/deps/litecoin) and just use litecoin headers directly.

uint64_t size() const noexcept { return bitset.size(); }

/// <summary>
/// Calculates the number of set bits that are smaller or equal to idx.

Choose a reason for hiding this comment

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

Code seems to suggest "Counts the number of set bits with indices lower than idx"

@Jogai
Copy link

@Jogai Jogai commented Mar 20, 2021

Files changed: 492. How is this even reviewable? Is no one concerned that some accidentally compromising code could slip in this way?

@losh11
Copy link
Member

@losh11 losh11 commented Mar 20, 2021

Files changed: 492. How is this even reviewable? Is no one concerned that some accidentally compromising code could slip in this way?

@Jogai Yes these changes are massive, and this is the libmw library alone. Right now Hector and others are just removing unused code, unused cryptos, comments etc. In the future David will likely split the changes into smaller pieces. As mentioned in the original comment David plans to release documents describing this process.

leaf_idx = leaf_idx.Next();
}

mmr::Index last_node = mmr::LeafIndex::At(unspent_leaf_indices.size()).GetNodeIndex();

Choose a reason for hiding this comment

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

unspent_leaf_indices.size() - 1 ?

std::for_each(
pegOutCoins.cbegin(), pegOutCoins.cend(),
[&pegOutAmounts](const PegOutCoin& coin) {
pegOutAmounts.insert({ coin.GetScriptPubKey(), coin.GetAmount() });

Choose a reason for hiding this comment

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

Are script pubkeys guaranteed to be unique?


StealthAddress Keychain::GetStealthAddress(const uint32_t index) const
{
assert(index < ULONG_MAX);

Choose a reason for hiding this comment

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

Why is index == ULONG_MAX disallowed?

assert(index < ULONG_MAX);
m_addressIndexCounter = std::max(m_addressIndexCounter, index);

PublicKey Bi = PublicKey::From(GetSpendKey(index));

Choose a reason for hiding this comment

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

Strange that the pubkey isn't added to m_pubkeys here

uint32_t pubkey_index = (uint32_t)m_pubkeys.size();
PublicKey pubkey = PublicKey::From(GetSpendKey(pubkey_index));

m_pubkeys.insert({ std::move(pubkey), pubkey_index });

Choose a reason for hiding this comment

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

Suggest rewriting the loop so that we don't regenerate keys that are already in the map, which can happen here if there are 'gaps' in the map, for example if the map contains 3 items, but the indexes are 0, 1 and 3.

Choose a reason for hiding this comment

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

Perhaps with the help of a second cache mapping indexes to key pairs. This cache could be used in GetStealthAddress & GetSpendKey.

SecretKey ephemeral_key = Random::CSPRNG<32>();
Output output = Output::Create(
blind,
EOutputFeatures::DEFAULT_OUTPUT,

Choose a reason for hiding this comment

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

Even for pegged-in outputs?

I noticed that EOutputFeatures::PEGGED_IN isn't set anywhere in the code.

#include <mw/crypto/Random.h>
#include <mw/models/tx/Kernel.h>

//TEST_CASE("Plain Kernel")

Choose a reason for hiding this comment

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

All tests are commented out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants