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

Move sc-network-gossip into Grandpa #28

Open
bkchr opened this issue Jan 20, 2023 · 5 comments
Open

Move sc-network-gossip into Grandpa #28

bkchr opened this issue Jan 20, 2023 · 5 comments
Labels
I4-refactor Code needs refactoring.

Comments

@bkchr
Copy link
Member

bkchr commented Jan 20, 2023

Grandpa is currently the only user of sc-network-gossip. Polkadot started using their own gossip implementation from the beginning. We also know that sc-network-gossip has certain bugs (like sending duplicate messages etc). We should move sc-network-gossip to Grandpa to remove the assumptions that the current implementation is reusable in different projects. While moving it to Grandpa we should fix the issues or maybe rewrite it completely. In the future we can maybe bring back some generic gossip implementation.

@bkchr
Copy link
Member Author

bkchr commented Jan 20, 2023

Beefy is also using it right now, but this shouldn't be such a big problem.

@kayabaNerve
Copy link

kayabaNerve commented Jan 21, 2023

I also use it as a consumer of Substrate. What would be the suggested migration path?

While I'm frustrated to now hear it's buggy, and am disappointed that wasn't documented, I'd rather it be fixed than moved internally. I'd especially rather it be fixed in-place if part of the discussion on moving it internally is fixing it/rewriting it.

My other question is are these bugs currently documented? I have not found stability issues once getting it working in the first place, though that was a pain... I also question how severe the bugs can be when it's still used currently in Grandpa.

@bkchr
Copy link
Member Author

bkchr commented Jan 21, 2023

What would be the suggested migration path?

You could fork the code.

My other question is are these bugs currently documented?

I don't know for sure. I also don't know exactly what are the issues, there are just assumptions around stuff not working properly, but AFAIK no one really looked into it.

@kayabaNerve
Copy link

I guess. While I fully ack Parity's ability to deprecate it, I'd still like to speak here as a counterpoint to deprecating it.

... that's... frustrating. I do understand how it can be difficult to identify exact issues in complex systems though, especially ones not so obvious. I didn't notice any in the issue tracker, and at a high level, haven't noticed any issues in usage. While I have noticed some peer disconnections over time, presumably due to downscoring, I wrote that off as minor latency issues causing a cumulative score failure. Not anything actually problematic, though that does potentially point to some underlying problem (just one hard to debug, as I acknowledge such issues likely are).

I won't bother to continue harping. I want to provide a counter, as a consumer, not be annoying to the people who build what I use. If there are further discussions on sc-network-gossip, I'd love to chime in, and if anyone knows of issues/complaints, even informally, I'd love the heads up.

@burdges
Copy link

burdges commented Jan 23, 2023

There are several different security goals for a gossip protocol, along with different techniques by which one might achieve them, ala our politeness logic strewn throughout polkadot, or the erlay set-reconciliation protocol in bitcoin's memepool.

We do think substrate should provide some gossip layer that clearly says what it achieves, vs what it does not achieve. It's hard to do this in 2023 though, given our own gossip mess, even in polkadot.

In other words, you won't necessarily give up (much) upstream maintenance support by forking this code because anything significantly altering this code might change its goals somewhat too.

@the-right-joyce the-right-joyce transferred this issue from paritytech/substrate Aug 24, 2023
@the-right-joyce the-right-joyce added I4-refactor Code needs refactoring. and removed I7-refactor labels Aug 25, 2023
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
* Bump jsonrpsee from `de7cbf2` to `a0bea41`

Bumps [jsonrpsee](https://github.com/paritytech/jsonrpsee) from `de7cbf2` to `a0bea41`.
- [Release notes](https://github.com/paritytech/jsonrpsee/releases)
- [Commits](paritytech/jsonrpsee@de7cbf2...a0bea41)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Fix imports.

* Update client code.

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Tomasz Drwięga <[email protected]>
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 8, 2024
* Bump jsonrpsee from `de7cbf2` to `a0bea41`

Bumps [jsonrpsee](https://github.com/paritytech/jsonrpsee) from `de7cbf2` to `a0bea41`.
- [Release notes](https://github.com/paritytech/jsonrpsee/releases)
- [Commits](paritytech/jsonrpsee@de7cbf2...a0bea41)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Fix imports.

* Update client code.

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Tomasz Drwięga <[email protected]>
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* Bump jsonrpsee from `de7cbf2` to `a0bea41`

Bumps [jsonrpsee](https://github.com/paritytech/jsonrpsee) from `de7cbf2` to `a0bea41`.
- [Release notes](https://github.com/paritytech/jsonrpsee/releases)
- [Commits](paritytech/jsonrpsee@de7cbf2...a0bea41)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Fix imports.

* Update client code.

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Tomasz Drwięga <[email protected]>
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* Bump jsonrpsee from `de7cbf2` to `a0bea41`

Bumps [jsonrpsee](https://github.com/paritytech/jsonrpsee) from `de7cbf2` to `a0bea41`.
- [Release notes](https://github.com/paritytech/jsonrpsee/releases)
- [Commits](paritytech/jsonrpsee@de7cbf2...a0bea41)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Fix imports.

* Update client code.

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Tomasz Drwięga <[email protected]>
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* Bump jsonrpsee from `de7cbf2` to `a0bea41`

Bumps [jsonrpsee](https://github.com/paritytech/jsonrpsee) from `de7cbf2` to `a0bea41`.
- [Release notes](https://github.com/paritytech/jsonrpsee/releases)
- [Commits](paritytech/jsonrpsee@de7cbf2...a0bea41)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Fix imports.

* Update client code.

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Tomasz Drwięga <[email protected]>
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* Bump jsonrpsee from `de7cbf2` to `a0bea41`

Bumps [jsonrpsee](https://github.com/paritytech/jsonrpsee) from `de7cbf2` to `a0bea41`.
- [Release notes](https://github.com/paritytech/jsonrpsee/releases)
- [Commits](paritytech/jsonrpsee@de7cbf2...a0bea41)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Fix imports.

* Update client code.

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Tomasz Drwięga <[email protected]>
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* Bump jsonrpsee from `de7cbf2` to `a0bea41`

Bumps [jsonrpsee](https://github.com/paritytech/jsonrpsee) from `de7cbf2` to `a0bea41`.
- [Release notes](https://github.com/paritytech/jsonrpsee/releases)
- [Commits](paritytech/jsonrpsee@de7cbf2...a0bea41)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Fix imports.

* Update client code.

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Tomasz Drwięga <[email protected]>
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 9, 2024
* Bump jsonrpsee from `de7cbf2` to `a0bea41`

Bumps [jsonrpsee](https://github.com/paritytech/jsonrpsee) from `de7cbf2` to `a0bea41`.
- [Release notes](https://github.com/paritytech/jsonrpsee/releases)
- [Commits](paritytech/jsonrpsee@de7cbf2...a0bea41)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Fix imports.

* Update client code.

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Tomasz Drwięga <[email protected]>
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 10, 2024
* Bump jsonrpsee from `de7cbf2` to `a0bea41`

Bumps [jsonrpsee](https://github.com/paritytech/jsonrpsee) from `de7cbf2` to `a0bea41`.
- [Release notes](https://github.com/paritytech/jsonrpsee/releases)
- [Commits](paritytech/jsonrpsee@de7cbf2...a0bea41)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Fix imports.

* Update client code.

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Tomasz Drwięga <[email protected]>
serban300 pushed a commit to serban300/polkadot-sdk that referenced this issue Apr 10, 2024
* Bump jsonrpsee from `de7cbf2` to `a0bea41`

Bumps [jsonrpsee](https://github.com/paritytech/jsonrpsee) from `de7cbf2` to `a0bea41`.
- [Release notes](https://github.com/paritytech/jsonrpsee/releases)
- [Commits](paritytech/jsonrpsee@de7cbf2...a0bea41)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Fix imports.

* Update client code.

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Tomasz Drwięga <[email protected]>
jonathanudd pushed a commit to jonathanudd/polkadot-sdk that referenced this issue Apr 10, 2024
* Bump jsonrpsee from `de7cbf2` to `a0bea41`

Bumps [jsonrpsee](https://github.com/paritytech/jsonrpsee) from `de7cbf2` to `a0bea41`.
- [Release notes](https://github.com/paritytech/jsonrpsee/releases)
- [Commits](paritytech/jsonrpsee@de7cbf2...a0bea41)

Signed-off-by: dependabot-preview[bot] <[email protected]>

* Fix imports.

* Update client code.

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Tomasz Drwięga <[email protected]>
liuchengxu added a commit to subcoin-project/polkadot-sdk that referenced this issue Sep 20, 2024
* Revisit max_sig_ops check

* Check block height in coinbase

* Check witness commitment

* Check header version

* Add height to Coin

* Check PrematureSpendOfCoinbase

* Move get_legacy_sig_op_count() into tx_verify module

* Check total_sig_ops_cost

* Use secrets.GITHUB_TOKEN instead of github.token

* Fix clippy

* Fix test

* Update docs

* Docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I4-refactor Code needs refactoring.
Projects
Status: Backlog 🗒
Status: backlog
Development

No branches or pull requests

4 participants