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

Add MimbleWimble support #2

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

webwarrior-ws
Copy link
Contributor

Implemented parsing and creation of MimbleWimble transactions, as well as Wallet type for keeping track of unspent coins. Added tests for implemented functionality.

@knocte
Copy link
Member

knocte commented Oct 12, 2023

I know you have squashed all commits into 1 in order to not have to curate your commit history to have all commits be green. But I prefer a not-all-green commit history for a PR (that I squash into one myself when merging) than just losing all those commits that you had @webwarrior-ws

@webwarrior-ws
Copy link
Contributor Author

I know you have squashed all commits into 1 in order to not have to curate your commit history to have all commits be green. But I prefer a not-all-green commit history for a PR (that I squash into one myself when merging) than just losing all those commits that you had @webwarrior-ws

What do you propose? To have all the original commits in the PR?

@knocte
Copy link
Member

knocte commented Oct 12, 2023

That's exactly what I said.

@knocte
Copy link
Member

knocte commented Oct 12, 2023

i.e. ...But I prefer a not-all-green commit history for a PR...

BigInteger.FromByteArrayUnsigned(arr).Mod(scalarOrder)

if k0 = BigInteger.Zero then
failwith "Failure. This happens only with negligible probability."
Copy link
Member

Choose a reason for hiding this comment

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

@webwarrior-ws WHAT???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's extremely unlikely that sha256 hash of some value will be all zeros or exactly scalarOrder.

member self.IsMine = self.AddressIndex <> Coin.UnknownIndex
member self.HasSpendKey = self.SpendKey.IsSome

static member Empty =
Copy link
Member

Choose a reason for hiding this comment

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

@webwarrior-ws can this be marked as private or internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it's used in tests. Unless I duplicate this method in tests.

Copy link
Member

Choose a reason for hiding this comment

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

don't duplicate it

@knocte
Copy link
Member

knocte commented Mar 21, 2024

@webwarrior-ws I guess this PR needs cleanup (e.g. squashing). But if you add LIP0006 work here, please add it as a separate commit.

Add types needed for MimbleWimble transactions and implement
serialization.

Made all MimbleWimble types implement ISerializable and
interface and Read static method. Reuse NBitcoin's
BitcoinStream for serialization and deserialization, but don't
use its IBitcoinSerializable interface because it mixes reading
and writing in one method.

Added new test project for MimbleWimble tests. Added test for
peg-in transaction.

Added property-based tests for serialization and
deserilaization.

Added test that deserilizes transaction generated and
serialized by modified litecoin test (see [1]).

Added tests for parsing peg-out transaction and MW to MW
(HogEx) transaction.

[1] https://github.com/litecoin-project/litecoin/blob/5ac781487cc9589131437b23c69829f04002b97e/src/libmw/test/tests/models/tx/Test_Transaction.cpp
@webwarrior-ws webwarrior-ws force-pushed the mimblewimble-squashed branch from 490c2ed to 5f5e144 Compare March 26, 2024 10:45
Add NBitcoin.Secp256k1 library because it has Schnorr
signature. As the library requires .netstandard 2.1,
make NLitecoin target it instead of 2.0.
Implement construction of range proofs.

Added a new test project that does differential tests for
zero-knowledge proof components that uses Secp256k1-ZKP.Net
library (libsecp256k1 wrapper) as reference.

Use Assert.Inconclusive if function is missing in libsecp256k1
on current platform.
Implement transaction builder and add tests for it.
Added Wallet type, which is responsible for key derivation,
rewinding of transaction outputs, and storage of rewound
outputs (coins), as well as creating transactions using our
keychain and spendable coins.
Ported test for key derivation and stealth address generation
using KeyChain.

Minor code style improvements.
Fixes in Wallet and TransactionBuilder mostly related to
public/private spend keys.
Added integration test for Wallet.
Update NBitcoin to version 7.0.13 because older NBitcoin
versions can't decode bech32 strings of length > 90, which is
needed for MimbleWimble address decoding.
Implemented decoding of MimbleWimble stealth addresses. Added
test.
Increase package's version to 0.2.0.
Implement "read-only" keychain, that allows for balance to be
checked without knowledge of wallet's seed (which would allow
for spending funds), only with private scan key and public
spend keys.
Addressed code review comments. Added latest Fsdk package in
order to use BetterAssert function.
Fixed transaction deserialization so that it now handles HogEx
transactions, which have mweb extension flag but don't contain
MW transaction. Added test for this case.
@webwarrior-ws webwarrior-ws force-pushed the mimblewimble-squashed branch from 44e944e to 9b278b1 Compare March 26, 2024 11:04
@webwarrior-ws
Copy link
Contributor Author

Squashed. CI for all commits is green now.

@knocte knocte changed the title Add MmimbleWimble support Add MimbleWimble support Mar 26, 2024
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.

2 participants