Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

one binary, multiple networks #8158

Closed
wants to merge 28 commits into from
Closed

Conversation

coryschwartz
Copy link

Related Issues

Proposed Changes

Allow the same binary to be used across multiple networks.

Additionally, parameters can be overridden by environment variables.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • All commits have a clear commit message.
  • The PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
    • area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
  • This PR has tests for new functionality or change in behaviour
  • If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
  • CI is green

@coryschwartz
Copy link
Author

coryschwartz commented Feb 19, 2022

WIP.

This will make it easier to change parameters, allowing the same binary to be used on multiple networks.

This removes the const, and in their place we access through getters.

I've submitted a similar proof-of-concept PR in the past and there was some consternation about removing the const's -- open to other ideas.

The justification for doing this is to ease operations. We want to be able to easily manage applications that should operate on multiple lotus networks without building and managing multiple binaries/images.

LOTUS_NETWORK=butterflynet ./lotus daemon

2022-02-19T03:21:26.919-0800	INFO	modules	modules/services.go:162	subscribing to pubsub topic /fil/blocks/butterflynet
2022-02-19T03:21:26.919-0800	INFO	messagepool	messagepool/messagepool.go:439	mpool ready
2022-02-19T03:21:26.920-0800	INFO	importmgr	imports/manager.go:62	sanity checking imports
2022/02/19 03:21:26 storess: Query
2022/02/19 03:21:26 storess: q.Prefix: 
2022/02/19 03:21:26 storess: q.KeysOnly: true
2022/02/19 03:21:26 storess: q.Filters: 0
2022/02/19 03:21:26 storess: q.Orders: 0
2022/02/19 03:21:26 storess: q.Offset: 0
2022-02-19T03:21:26.921-0800	INFO	importmgr	imports/manager.go:99	sanity check completed	{"broken": 0, "total": 0}
2022-02-19T03:21:26.922-0800	INFO	dt-impl	impl/impl.go:145	start data-transfer module
2022-02-19T03:21:26.931-0800	INFO	markets	loggers/loggers.go:55	module ready	{"module": "discovery"}
2022-02-19T03:21:26.933-0800	INFO	markets	loggers/loggers.go:55	module ready	{"module": "client data transfer"}
2022-02-19T03:21:26.936-0800	INFO	markets	loggers/loggers.go:55	module ready	{"module": "storage client"}
2022-02-19T03:21:26.937-0800	INFO	markets	loggers/loggers.go:55	module ready	{"module": "retrieval client"}
2022-02-19T03:21:31.639-0800	INFO	basichost	basic/natmgr.go:84	DiscoverNAT error:no NAT found
2022-02-19T03:21:31.917-0800	INFO	peermgr	peermgr/peermgr.go:210	connecting to bootstrap peers

@coryschwartz coryschwartz changed the title one binary, multiple networks WIP: one binary, multiple networks Feb 19, 2022
@travisperson
Copy link
Contributor

I think this is a good first pass at this, nothing jump out at me in regard to setting up or managing networks. I think we should discuss this with @arajasek.

@coryschwartz coryschwartz changed the title WIP: one binary, multiple networks one binary, multiple networks Mar 25, 2022
@coryschwartz
Copy link
Author

This allows you to switch networks with the same binary.

There are lots of of reasons it would be good to have a binary that can switch networks, particularly, this makes it easier to operate multiple netowrks with the same packages, docker images, etc.

Additionally, this adds the ability to specify a network definition file aside from the built-in file. Allowing external network definitions will help build new netowrks on the fly with custom parameters.

Currently, this is configured using only environment variables -- I'm not really stuck on this design. I considered re-writing this as a dependency injection, and could be convinced that this would be a better choice.

The current design takes previously constant values and instead exports them as getters only, so they cannot be easily changed at runtime.

@coryschwartz coryschwartz marked this pull request as ready for review March 25, 2022 01:27
@coryschwartz coryschwartz requested a review from a team as a code owner March 25, 2022 01:27
@jennijuju
Copy link
Member

woo this looks cool

@magik6k magik6k self-requested a review April 16, 2022 10:49
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Conflicts/tests need fixing, but this looks really good

Comment on lines -241 to -243
och := build.UpgradeClausHeight
build.UpgradeClausHeight = 0
t.Cleanup(func() { build.UpgradeClausHeight = och })
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably can just drop this entire test

Copy link
Contributor

@travisperson travisperson left a comment

Choose a reason for hiding this comment

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

This is a pretty big changes and will require communication.

This PR doesn't touch the Makefile. However, a lot of the targets are no longer needed and should be removed (all the network builds).

The BuildType + version string should also be addressed.

lotus/build/version.go

Lines 8 to 37 in 97b92cd

const (
BuildDefault = 0
BuildMainnet = 0x1
Build2k = 0x2
BuildDebug = 0x3
BuildCalibnet = 0x4
BuildInteropnet = 0x5
BuildButterflynet = 0x7
)
func BuildTypeString() string {
switch BuildType {
case BuildDefault:
return ""
case BuildMainnet:
return "+mainnet"
case Build2k:
return "+2k"
case BuildDebug:
return "+debug"
case BuildCalibnet:
return "+calibnet"
case BuildInteropnet:
return "+interopnet"
case BuildButterflynet:
return "+butterflynet"
default:
return "+huh?"
}
}

It would probably be good to keep this version reporting, but make it defined by the LOTUS_NETWORK / LOTUS_NETWORK_DEFINITION_FILE. This will allow people to ask the daemon what network it's talking too, we shouldn't remove this piece of information from users. It's an important bit to gather when users fill out issues.

BuildType |= BuildDebug
}

// NOTE: Also includes settings from params_2k
Copy link
Contributor

Choose a reason for hiding this comment

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

Deleting this file breaks make debug, and no longer does anything. We need to keep the functionality that this file brings.

  1. Disabling PoSt
  2. Adding the +debug to the version string (this is done by the BuildType |= BuildDebug).

Copy link
Author

Choose a reason for hiding this comment

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

oo, good point.

I was hoping debug networks could be a runtime rather than build time decision. But there are some things that aren't ready for that, and I don't really want to make this PR any bigger than it already is.

I've added a params_debug back in. It sets the SlashablePowerDelay, Insecure PoSt, and sets the "BuildType" to debug.

@codecov
Copy link

codecov bot commented Apr 25, 2022

Codecov Report

Merging #8158 (3c6db2b) into master (88f8c7d) will decrease coverage by 0.00%.
The diff coverage is 66.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8158      +/-   ##
==========================================
- Coverage   40.86%   40.86%   -0.01%     
==========================================
  Files         686      686              
  Lines       75710    75784      +74     
==========================================
+ Hits        30939    30966      +27     
- Misses      39445    39466      +21     
- Partials     5326     5352      +26     
Impacted Files Coverage Δ
chain/gen/slashfilter/slashfilter.go 40.00% <0.00%> (ø)
chain/store/messages.go 73.58% <0.00%> (ø)
chain/vm/gas.go 79.16% <ø> (ø)
chain/vm/syscalls.go 39.07% <0.00%> (ø)
chain/vm/vm.go 62.16% <0.00%> (-0.54%) ⬇️
cli/sync.go 35.79% <0.00%> (ø)
cmd/lotus-health/main.go 16.26% <0.00%> (ø)
cmd/lotus-miner/info.go 60.50% <0.00%> (ø)
cmd/lotus-pcr/main.go 0.00% <0.00%> (ø)
cmd/lotus-seed/genesis.go 0.00% <0.00%> (ø)
... and 68 more

@coryschwartz
Copy link
Author

This is a pretty big changes and will require communication.

This PR doesn't touch the Makefile. However, a lot of the targets are no longer needed and should be removed (all the network builds).

The BuildType + version string should also be addressed.

lotus/build/version.go

Lines 8 to 37 in 97b92cd

const (
BuildDefault = 0
BuildMainnet = 0x1
Build2k = 0x2
BuildDebug = 0x3
BuildCalibnet = 0x4
BuildInteropnet = 0x5
BuildButterflynet = 0x7
)
func BuildTypeString() string {
switch BuildType {
case BuildDefault:
return ""
case BuildMainnet:
return "+mainnet"
case Build2k:
return "+2k"
case BuildDebug:
return "+debug"
case BuildCalibnet:
return "+calibnet"
case BuildInteropnet:
return "+interopnet"
case BuildButterflynet:
return "+butterflynet"
default:
return "+huh?"
}
}

It would probably be good to keep this version reporting, but make it defined by the LOTUS_NETWORK / LOTUS_NETWORK_DEFINITION_FILE. This will allow people to ask the daemon what network it's talking too, we shouldn't remove this piece of information from users. It's an important bit to gather when users fill out issues.

Updated the version string so it includes the network name.

It also occurs to me that anyone could create a network that matches a well-known built-in network, so I included a hash of the genesis there as well.

@vyzo
Copy link
Contributor

vyzo commented Apr 26, 2022

we will need to rethink bundle loading for nv16 with this.

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

I think we can land this if we get the backwards compatibility working

Makefile Outdated
Comment on lines 73 to 83
2k: GOFLAGS+=-tags=2k
2k: build-devnets

calibnet: GOFLAGS+=-tags=calibnet
calibnet: build-devnets

butterflynet: GOFLAGS+=-tags=butterflynet
butterflynet: build-devnets

interopnet: GOFLAGS+=-tags=interopnet
interopnet: build-devnets
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to keep those for compat for now - just changing the default network in the binary.

@vyzo
Copy link
Contributor

vyzo commented May 6, 2022

Can we delay until after m1 ships, so that we figure out the bundling strategy?
Right now it would create irreconcilable conflicts with nv16.

@ianconsolata
Copy link
Contributor

How is this going to affect #9625?

@Kubuxu
Copy link
Contributor

Kubuxu commented Nov 15, 2022

How is this going to affect #9625?

It would no longer be required.

@rjan90
Copy link
Contributor

rjan90 commented Sep 10, 2024

If somebody else wants this, it is probably better to start from scratch given the long list of conflicts, and how outdated this PR is by now. Closing for now.

@rjan90 rjan90 closed this Sep 10, 2024
@rjan90 rjan90 deleted the feat/multi-network-binary branch February 3, 2025 08:52
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants