lnp-bp / rust-lnpbp Goto Github PK

Bitcoin normally uses VarInt to represent the number of transaction outputs that may be contained in a given transaction and 32-bit integer to reference specific transaction output from other's transaction input. While with the given block size limit transaction containing more than 2^16 outputs can't fit the block, it's better not to diverge from bitcoin data structure. The original reason why 16-bit integers were introduced is to save some storage space for RGB client-validated data; however bitcoin transaction output single-use seals are more generic standard and RGB consensus commitment encoding does not require storage space optimization. RGB P2P protocols and vendor-specific RGB storage may still use 16-bit integers; however this should not be required by the LNP/BP standards.

This will involve modification of rust-bitcoin and rust-lightning libraries and related PRs to them

Upstream PRs:

• teawithsand/torut#7
• teawithsand/torut#6
• teawithsand/torut#5

• commits to schema
• contains schema in network serialization format

• Wrappers
• AsSlice
• Useful macro

• Bipolar
• Internet
• Service

And remove "common" mod

Some points of special attention:

Running cargo fmt on the current (bd1bd41) master produces a huge diff:

48 files changed, 936 insertions(+), 666 deletions(-)

I won't make a PR now because it would make merging stuff a mess, but we should consider running it when we don't have any big PR open

Here:

According to LNP-BP/LNPBPs#33

so the code compiles with #![deny(missing_docs)]

Right now commit-encode procedure for RGB nodes just uses strict serialization of all the data. Move to the merklization and conceal algorithm for both metadata and assignments.

Using library https://github.com/apoelstra/rust-miniscript.

Note: may require PR's to Miniscript and most probably significant rust-bitcoin modification/adoption. Related issue: rust-bitcoin/rust-bitcoin#393

• Use single_use_seals andbp::seals txou_seals (while removing bp::seals::tx_graph in favor of validation::TxResolver
• Implement lnpbps::lnpbp2 and lnpbps::lnpbp3, use them from anchors instead of direct DBC use

Related:

Matching LNP-BP/LNPBPs#45

Especially with Conceal-related matters

• Internet2 networking with TCP & ZMQ
• Feature management
• Noise encryption
• LN-specific data types (only BigInt left)
• Messages: BOLT-1
• Messages: BOLT-2
• Messages: BOLT-7
• LN-specific data types encoding

While with those data structures that participate in commitments we can't use versioning without risking complex hard-fork issues under client-side validation paradigm, we can (and should) have versioning for network/storage serialized data, like consignments and disclosures.

TODO: Add version field to consignments and disclosures, defaulting now to 1

BOLT-9 is implemented in multiple parts of rust-lightning. It will be useful to create a streamlined abstraction to that functions

@afilini was right #24 (comment) that we need to add support of TORv2 addresses additionally to TORv3: at least they are a part of Lightning Network gossip protocol https://github.com/lightningnetwork/lightning-rfc/blob/master/07-routing-gossip.md#the-node_announcement-message and we have to support them in LNP part of the library.

Basically, we need to duplicate all TORv3-related code and adjust it to TORv2-specific types in here: https://github.com/LNP-BP/rust-lnpbp/blob/master/src/common/internet.rs. Also there is a need to add feature requirement to Cargo.toml torut crate as pointed out here: #24 (comment)

Do not forget to modify code at the following locations:

https://doc.rust-lang.org/1.26.2/unstable-book/language-features/repr-transparent.html

If this has been already discussed or kept here for future purposes, feel free to close this issue.

ScriptPubkeyComposition is a major input in the high level TxCoinatianer, which is subsequently passed down into smaller containers. So it feels to me that a better location for ScriptPubkeyComposition is with the other protocol level primitive types like Proof and ScriptInfo. Currently, its located in dbc/scriptpubkey.rs.

Thus proposing to reallocate this enum with other primitive types.

Upstream dependencies:

• rust-bitcoin/rust-bitcoin#481
• #102

Make serialize its root structure, remove old unnecessary data types; move multimessage commitments to CMT

BOLT-1 defines messages that peer nodes exchange before any channel establishment. The data related to these messages are scattered throughout rust-lightning library. It will be useful to abstract them into a single mod in the lnp mod of the library

We need to support wide range of networks (including Liquid, LN) and create a foundation for generic asset concept (that includes chain-based assets and RGB assets).

Draft concept:

/// P2P network magic number: prefix identifying network on which node operates
pub type P2pMagic = u32;
/// Magic number prefixing Pubkey or Prvkey data according to BIP32 spec
pub type Bip32Magic = u32;

#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[non_exhaustive]
#[repr(u8)]
pub enum ChainFormat {
    Bitcoin = 0,
    Elements = 1,
}

#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[repr(u8)]
pub enum AssetLayer {
    Layer1and2,
    Layer2and3,
}

#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct AssetParams {
    pub ticker: &'static str,
    pub unit_of_accounting: &'static str,
    pub indivisible_unit: &'static str,
    pub divisibility: u64,
    pub asset_id: AssetId,
}

#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct ChainParams {
    pub name: &'static str,
    pub p2p_magic: P2pMagic,
    pub genesis_hash: BlockHash,
    pub bip32_pubkey: Bip32Magic,
    pub bip32_prvkey: Bip32Magic,
    pub bip70_name: &'static str,
    pub bip173_prefix: &'static str,
    pub p2p_port: u16,
    pub rpc_port: u16,
    pub ln_height: u32,
    pub format: ChainFormat,
    pub dust_limit: u64,
    pub native_asset: AssetParams,
    pub testnet: bool,
    pub pow: bool,
}

/// A set of recommended standard networks. Differs from bitcoin::Network in
/// ability to support non-standard and non-predefined networks
#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
#[non_exhaustive]
#[repr(u32)]
pub enum Chains {
    Mainnet,
    Testnet3,
    Regtest(BlockHash),
    Signet,
    SignetPrivate(BlockHash),
    LiquidV1,
    Other(ChainParams),
}

Use statements containing wildcards (e.g. use foo::*) have a significant disadvantage: it makes it very hard to understand where an item is coming from, especially for new people.

For example, when written as:

use foo::*;
use bar::*;

let baz = Baz::new();

It's impossible to know where Baz comes from without using search tool. However from:

use foo::Something;
use bar::Baz;

let baz = Baz::new();

We can easily see Baz is in bar.

Further, this could cause various issues like two developers adding an item with same name in different modules causing conflict in a third module after merge, which can not happen with explicit use statements.

As an alternative, limit the scope of asterisks to where they're needed. Eg.g sometimes it's useful to use LongEnumNameWithManyVariants::*; at the top of a function containing a huge match.

https://github.com/LNP-BP/rust-lnpbp/blob/master/src/bp/tagged256.rs#L36

The macro is unused so I can't say for sure what it's meant to do, but that looks wrong to me. From the docs: "This macro will yield an expression of type &'static str which is the stringification of all the tokens passed to the macro. No restrictions are placed on the syntax of the macro invocation itself.".

Kinda dangerous because it wouldn't make anything fail in an obvious way, but it would be a mess to remove in the future.

I would just remove the stringify and concat $prefix and $tag.

Anyone may take any module and add as much tests as you like :)

When we have network in Schema, this will result in that the same schema will have a different SchemaId on different networks (mainnet, testnet), which is undesirable. In fact, network is an asset-specific thing (i.e. genesis is more natural place), and not smart contract category-specific.

Next, Genesis must include full ChainParams data structure, not just a network ID. This will allow RGB to operate on top of any future chain (like new federated sidechains) without requiring library updates.

Related issues:

• #102 Refactor bp::Network into ChainParams
• #95 Use ChainParams instead of Network

I noticed that InetAddr as well as other types use String to report errors. This is generally viewed as anti-pattern, especially in library code. I suggest to use custom error types.

We can decrease boilerplate in the early stage of development by using crates thiserror and anyhow using this pattern:

#[derive(Debug, Error)]
#[thiserror(transparent)] // NOT to be confused with repr(transparent)!!! completely different things
struct InetAddrError(anyhow::Error);

This way we will not waste time writing custom enums that we aren't confident about, but avoid running into technical debt of rewriting code to use something else than strings later. The dependencies can be removed at any point and replaced with manual implementation without loss of features or compatibility.

Important: we must not impl From<anyhow::Error> for InetAddrError as that would make the type forward-incompatible.

I might be able to help with this.

Genesis commits to ChainHash (hash of the genesis block of the underlying blockchain), however it contains (for P2P networking purposes) information about the target blockchain (name etc) called ChainParam. This allows not to hardcore list of supported blockchains (main, test, signet, regtest, sidechains, test sidechains...) into each and all client-level software. However, ChainParam may evolve, so to avoid compatibility issues and allow extensibility w/o RGB contract change we
will serialize chain parameters in all known/legacy formats for compatibility:

genesis := some_data, chain_params, rest_of_data
chain_params := no_of_encodins, chain_param_encoding*
chain_param_encoding := len, bytes*

TODOs

• Make all PRs in upstream crates merged
• Get version bump in upstream crates after the last PR released
• Complete code docs #81
• Remove all dead code

  rust-lnpbp/src/lib.rs

  Line 33 in 6010402

  // TODO: when we will be ready for the release #![deny(dead_code)]

• Remove re-exporting of bitcoin crates

  rust-lnpbp/src/lib.rs

  Line 64 in 6010402

  // TODO: Remove re-exporting of bitcoin crates on release

Upstream dependencies requiring attention

• grin_secp256k1zkp
• bitcoin_hashes
• rust-bitcoin
• rust-lightning
• miniscript

List of opened upstream PRs

• All PSBT and BIP-32-related PRs from epic rust-bitcoin/rust-bitcoin#473
• (optional) All bitcoin_num related PRs from epic rust-bitcoin/rust-bitcoin#467
• Taproot-related PRs
• rust-bitcoin/bitcoin_hashes#90
• rust-bitcoin/bitcoin_hashes#89
• rust-bitcoin/rust-miniscript#70
• rust-bitcoin/rust-bitcoin#388
• rust-bitcoin/rust-bitcoin#480
• rust-bitcoin/rust-bitcoin#435
• lightningdevkit/rust-lightning#623
• rust-bitcoin/rust-bitcoin#395
• rust-bitcoin/rust-bitcoin#387
  ...

This causes the library to download and compile the crate even if it's not being used.

Suggestion: make the crate optional and use its name instead of feature in #[cfg()]

There seems to be an awkward import here with pub use elliptic_curve::EllipticCurve;

Was this intentional? Why specifically import EllipticCurve like this and not do the same for SignatureAlgorithm and PointSerialization?

Recent commits with updates to upstream dependencies has broken consignment test: it now leads to rust compiler stack overflow. We need to investigate and test this issue

Implement phase I from LNP-BP/LNPBPs#57

Bulletproofs from the library must be used instead of Borromean ring signatures like in current Liquid/Elements version

https://github.com/LNP-BP/rust-lnpbp/blob/master/src/bp/strict_encoding.rs