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

Substrate Complaints #28

Closed
kayabaNerve opened this issue Jul 2, 2022 · 28 comments
Closed

Substrate Complaints #28

kayabaNerve opened this issue Jul 2, 2022 · 28 comments
Labels
discussion This requires discussion improvement This could be better node

Comments

@kayabaNerve
Copy link
Member

kayabaNerve commented Jul 2, 2022

I wanted to open a running list of issues with Substrate I'd like to tackle, yet need some level of review before doing so is possible.

  • SS58. bech32m should be easier to communicate.
  • Multiaddress? I'd like to remove this solely for RistrettoPoint (or Enum u64/RP).
  • U128 balances. U64 should always be fine for Serai. While Ethereum is substantially larger, we can truncate, and U64 will be far more performant.
  • Usage of merlin for Schnorr signatures. merlin is an overtly complicated specification and it currently doesn't work on big-endian CPUs, for whatever reason. The question becomes at what point do our changes break the ecosystem so much we're the overtly complicated group.
  • Support for ECDSA/Ed25519. That's solely valuable from an ink context, not valuable for Substrate itself.
  • Emojis in log statements.
  • Lack of auditing.
@kayabaNerve kayabaNerve added improvement This could be better discussion This requires discussion protocol labels Jul 2, 2022
@kayabaNerve kayabaNerve added this to the Testnet milestone Jul 20, 2022
@kayabaNerve
Copy link
Member Author

Labeling testnet as this notably effects integration.

@kayabaNerve
Copy link
Member Author

It uses --dev, instead of --devnet. I would've preferred --devnet for two reasons.

  1. mainnet/testnet consistency
  2. My previous project used the phrase devnet

So why aren't we on devnet right now? Because --dev is a flag, setting a network ID dev, yet renaming the network ID to devnet doesn't update that option because its defined in a Substrate crate which assumes consistency. That entire crate would have to be updated to do this. It's the weirdest comment of both flexibility and none at all.

@kayabaNerve
Copy link
Member Author

The next steps for https://github.com/serai-dex/substrate would be, in order:

1a) Remove all crates not relevant to Serai. I truly don't care to spend the time and effort to maintain them.
1b) Remove features not relevant to Serai from crates relevant to Serai, with a goal on minimizing dependencies (not on minimizing size).
2) Fork or merge in repos from other sections of parity, such as parity-common.
3) Merge crates as possible. Instead of kvdb and kvdb-rocksdb and kvdb-paritydb and kvdb-tests and... just do kvdb with feature flags, or follow the policy from 1b (depends if we want to promote an alternative ecosystem, like we do with our crypto libs, or just focus on Serai).

This comment details the larger plan to drop Parity Substrate for Serai Substrate. While relevant to the above, this will probably end up as two issues. The above will be pre-mainnet, the latter will be post.

@kayabaNerve
Copy link
Member Author

I believe we did successfully drop Multiaddress/Multisignature in 28308df. U128 balances were replaced with U64 in one of our earliest Substrate commits, at least regarding the core runtime itself.

@kayabaNerve
Copy link
Member Author

As part of implementing Tendermint, I learned various pallets define their own key types. While I have no idea why, the Tendermint pallet had to define its keys, and for some reason, its Ristretto key is distinct from the standard Ristretto key. The other pain is the classic Ristretto key isn't even Ristretto, yet [u8; 32].

It's only as largely changing /substrate that we'd be able to ensure there's only one key type, Ristretto, by purging the others from the dependency tree.

@kayabaNerve
Copy link
Member Author

kayabaNerve commented Nov 8, 2022

I previously accused Substrate of having 'horrible' package layouts. I now see exactly why it's written the way it is, with three crates for everything. I believe primitives and pallet can be merged, yet that's of marginal benefit. They cannot be due to pallets which cross-reference each other.

Instead of criticizing that aspect, I'd instead like to criticize their folder organization.

  • primitives/x
  • pallet/x
  • client/x

Given how many crates exist under each folder, this causes massive travel distance to jump between them. Instead, I'd like to propose x/[primitives, pallet, client].

@TheArchitect108 TheArchitect108 added the wontfix This will not be worked on label Nov 15, 2022
@kayabaNerve kayabaNerve added node and removed wontfix This will not be worked on protocol labels Nov 15, 2022
@kayabaNerve
Copy link
Member Author

I'd like to remove authoring_version from the runtime, as it's pretty purely vestigial at this point. So is SelectChain, which Tendermint can't provide a correct implementation for, and Substrate doesn't use internally, yet Substrate still requires.

@kayabaNerve
Copy link
Member Author

Per #180, we no longer have ink and can purge ECDSA/Ed25519 without a second thought.

@kayabaNerve
Copy link
Member Author

Substrate uses xx hash for pallet/container labels. While this is practically fine, those can have collisions crafted and a malicious PR could introduce such a fault. We should move that to Blake/find a way to check at compile time no items in pallets conflict with another.

@kayabaNerve kayabaNerve removed this from the Protonet 0 milestone Dec 8, 2022
@kayabaNerve
Copy link
Member Author

kayabaNerve commented Jan 23, 2023

wasm64 is far more recent, arguably unstable, and only has a 64-bit addressing space. wasm32 still has 64-bit integers, so I'm no longer concerned about the efficiency.

@kayabaNerve
Copy link
Member Author

kayabaNerve commented Mar 26, 2023

Substrate features wasmtime yet wasmer seems to offer better performance/flexibility?

@kayabaNerve
Copy link
Member Author

https://github.com/paritytech/substrate/issues/ 10579
https://github.com/paritytech/substrate/issues/ 1252
are two issues which will hopefully be resolved by launch.

I'm also keeping an eye on https://github.com/paritytech/substrate/issues/ 9162.

@kayabaNerve
Copy link
Member Author

serai-dex/substrate@a4cb497 did remove the usage of Ed25519 in GRANDPA for sr25519.

@kayabaNerve
Copy link
Member Author

serai-dex/substrate@638c672 updated substrate to the latest schnorrkel/merlin, removing ~12 unnecessary dependencies.

@kayabaNerve
Copy link
Member Author

substrate 1252 was merged and we now no longer need nightly, it's solely preferred for additional fmt control/checks.

@kayabaNerve
Copy link
Member Author

schnorrkel does use the -ng variants of dalek crates. Ideally we'd simplify to just having the stock variants, especially now that curve25519-dalek is being maintained again.

@kayabaNerve
Copy link
Member Author

cargo machete reports a lot. While some of these are false positives (due to pallet macro expansion), I assume a good amount aren't.

@kayabaNerve
Copy link
Member Author

Substrate adds __Ignore variants to Call/Error. I have no idea why.

@kayabaNerve
Copy link
Member Author

The recent polkadot-v1.0.0 resolves authoring_version.

I've been removing it's 'over-specification' of crate versions. TIL x.y.z == ^x.y.z, not =x.y.z. This was a misunderstanding on my end. I'll make a tool to restore its specifications and minimize the merge work accordingly.

@kayabaNerve
Copy link
Member Author

The Freeze API, IMO, is fundamentally unsafe and should be removed.

@kayabaNerve
Copy link
Member Author

I'd like to look at removing the metadata. I have a lot of commentary on it, yet I'll leave it for another day at the time.

@kayabaNerve
Copy link
Member Author

primitives/metadata-ir is the relevant crate.

@kayabaNerve
Copy link
Member Author

kayabaNerve commented Aug 23, 2023

Do we want to change the Schnorrkel context string from substrate to Serai?

@kayabaNerve
Copy link
Member Author

Substrate silelntly drops events after 4 billion. Should we panic there?

@kayabaNerve
Copy link
Member Author

Looks like we're rewriting balances + assets into a single new pallet.

We also need to ensure we're only using RocksDB, not the WIP parity-db.

@kayabaNerve
Copy link
Member Author

All events per-block are stores as a single giant blob. While I assume this is for performance, as it means no new tree nodes are needed, it also creates a massive blob to get events from after-the-fact, and harms the hell out of light clients.

We should index per event, per n events, or per transaction (or per transaction, per event).

@kayabaNerve
Copy link
Member Author

We need to review how equivocation is handled. GRANDPA was moved to Ristretto, yet any code in the runtime wasn't...

@kayabaNerve
Copy link
Member Author

Closed for #379.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion This requires discussion improvement This could be better node
Projects
None yet
Development

No branches or pull requests

2 participants