[backport to v0.37.x] config: add bootstrap peers (tendermint#9680) by cason · Pull Request #1019 · cometbft/cometbft

cason · 2023-06-23T07:00:45Z

Closes #1016.

Reviewers, please help with the .changelog/ entry. The remain is just a simple backport from main.

jmalicevic · 2023-06-27T08:05:44Z

config/toml.go


+# Comma separated list of peers to be added to the peer store
+# on startup. Either BootstrapPeers or PersistentPeers are
+# needed for peer discovery


Maybe write down what the difference is between them although most people already know.

The affirmation is not even true. One node can discovery peers by only having SeedNodes config parameter set.

So, again, should we make those changes here, which is a backport PR, or in the original content on main?

So, here there are several points which are not clear:

A node can discovery peers provided it has any address on its address book (BootrstapPeers flag ensures this), any peer configured as persistent, or any address configured as seed node

Persistent peers is not meant for peer discovery. It is mostly used by validator nodes that don't run the PEX reactor, namely, don't participate on the peer discovery protocol

There is no peer store. There is an address book with addresses of potential peers

hvanz

LGTM

jmalicevic

Minor comment but LGTM.

sergio-mena · 2023-06-27T08:52:43Z

node/node.go

 		return nil, fmt.Errorf("could not create addrbook: %w", err)
 	}

+	for _, addr := range splitAndTrimEmpty(config.P2P.BootstrapPeers, ",", " ") {


As this is hard to unit-test (code in Node usually is), would it make sense to add support for this in the e2e manifest (no need to touch the generator) so we can test this in, e.g., networks/ci.toml?

I didn't get the point here.

The addresses here are directly passed to the address book, the address book unit tests should cover this. If not, the PEX reactor unit test, which requires setting up the address book, should also cover this.

I didn't get the point here.

As a general rule, when we add production code, we need to add some (unit) test code that hits the new code.
Here, I think unit testing would be cumbersome and pointless (but I might be wrong, as I didn't look around into UTs that test neighboring code... it may be simpler), so I was suggesting to add the config option as part of networks/ci.toml so that it's part of e2e

The addresses here are directly passed to the address book, the address book unit tests should cover this. If not, the PEX reactor unit test, which requires setting up the address book, should also cover this.

Can you point at the test cases that are testing this new code?

There is no test unit covering this flag specifically. In particular because this is not a flag that is considered by the p2p layer.

What I meant is that there are test cases covering the addrBook.AddAddress() method, which is a very basic method of the PEX implementation, that is invoked for all addresses parsed from this config flag.

so I was suggesting to add the config option as part of networks/ci.toml so that it's part of e2e

Ok, I will take a look on this file.

Ok, found it, but this is a specific config triggering some scenarios. I don't see the use of this flag in the network setup/topology enforced in those runs. Actually, those runs do not cover the user case of this new flag, as node rely on persistent peers and a seed node for establishing connections. The option there would be to replace a persistent peer in a given node by a bootstrap peer, but this would mean changing the semantics of the setup, as the node will not necessarily connect to the configured bootstrap peer, as it can retrieve other addresses from the nodes configured as persistent or seeds beforehand.

OK, if you think it makes no sense to test this using e2e, then we're back with unit tests.

I took a closer look at relevant existing UTs. There is TestNodeNewNodeCustomReactors which is testing the NewNode function where your new code is. So I see several options:

You can create a similar test case, next to it, where you modify the config before passing it to NewNode so that in contains relevant bootstrap peer config. The easiest is to feed incorrect config, to exercise the error paths introduced by the new code (where you are returning an error).

For testing the happy path, I think it's best to modify TestP2PConfig to include correct bootstrap config, and then re-running all UTs and make sure they all pass. Alternatively, you can just test the happy path as part of the new UT where you're testing the error paths

cason · 2023-06-27T11:06:51Z

So, I am ok to apply the suggested changes provided they are not part of this PR. This PR is meant to backport the code and operation present on main and v0.38.x branches to the v0.37.x branches.

I can take the opportunity to update the documentation (this includes comments on source files and descriptions on config files) so that they are more precise and clear. But I would rather do that as part of a different issue/PR, and first on main, then on release branches.

cason · 2023-06-27T11:44:49Z

I've created this new issue to handle updates on the p2p parameters: #1038

thanethomson · 2023-06-27T11:49:22Z

.changelog/unreleased/features/9680-config-bootstrap-ppers.md

@@ -0,0 +1,3 @@
+- `[config]` Introduce `BootstrapPeers` to the config to allow


Suggested change

- `[config]` Introduce `BootstrapPeers` to the config to allow

- `[config]` Introduce `p2p.bootstrap_peers` to the config to allow

Operators don't see the BootstrapPeers variable - they see the configuration file entry.

thanethomson · 2023-06-27T11:50:19Z

UPGRADING.md

+* A new config field, `BootstrapPeers` has been introduced as a means of
+  adding a list of addresses to the addressbook upon initializing a node. This is an
+  alternative to `PersistentPeers`. `PersistentPeers` shold be only used for
+  nodes that you want to keep a constant connection with i.e. sentry nodes


Suggested change

* A new config field, `BootstrapPeers` has been introduced as a means of

adding a list of addresses to the addressbook upon initializing a node. This is an

alternative to `PersistentPeers`. `PersistentPeers` shold be only used for

nodes that you want to keep a constant connection with i.e. sentry nodes

* A new config field, `p2p.bootstrap_peers` has been introduced as a means of

adding a list of addresses to the addressbook upon initializing a node. This is an

alternative to `p2p.persistent_peers`. `p2p.persistent_peers` should be only used for

nodes that you want to keep a constant connection with i.e. sentry nodes.

thanethomson · 2023-06-27T11:51:28Z

config/toml.go

 seeds = "{{ .P2P.Seeds }}"

+# Comma separated list of peers to be added to the peer store
+# on startup. Either BootstrapPeers or PersistentPeers are


Suggested change

# on startup. Either BootstrapPeers or PersistentPeers are

# on startup. Either bootstrap_peers or persistent_peers are

The configuration file has no notion of BootstrapPeers - this is the Go code field name. The representation in the config file is bootstrap_peers.

sergio-mena · 2023-06-27T17:26:26Z

This PR is meant to backport the code and operation present on main and v0.38.x branches to the v0.37.x branches.

I see. Then the UTs were missing on the initial PR. Please add the UTs I am requesting on main (in a different PR)

cason · 2023-06-28T07:10:05Z

node/node.go

+		if err != nil {
+			return nil, fmt.Errorf("invalid bootstrap peer address: %w", err)
+		}
+		err = addrBook.AddAddress(netAddrs, netAddrs)


I've just realize that the source address (second parameter) should not be that one.

In fact, when we manually add addresses to the address book, the source should be the node itself. Refer to the implementation spec (point 3. referring to the source address) and to the way we add addresses of configured persistent peers on the switch.

cason · 2023-06-28T07:13:54Z

I think we should block (if not drop) this PR, since the implementation of this feature in the main branch has a lot of (minor) problems, listed by the reviewers.

For instance:

The documentation of the new configuration parameter is imprecise and it is not following the correct naming for config/toml file parameters
The way we are adding the parsed addresses to the address book is incorrect (improper source address)
We don't have tests checking the operation of the introduced configuration parameter

While none of those problems are critical, it would be good to fix them on main to then backport a cleaner and safer implementation to the feature branches.

sergio-mena · 2023-06-28T09:06:26Z

@cason I think I agree with you overall. Here's the next steps I suggest

As this is a backport (I hadn't realized until yesterday, sorry),
- either we backport it as-is (with no changes w.r.t. the original patch),
- or we revert the original on main (and any other branch that may contain it)
and then, just after, we open a PR that addresses the problems the original patch has (on main then backport)

Also, this PR was in response to a user's request wasn't it?

cason · 2023-06-28T09:20:41Z

Also, this PR was in response to a user's request wasn't it?

Yes, #1016

sergio-mena · 2023-06-29T13:22:39Z

Then, whichever option we take, we need to drive it to completion, so #1016 can be considered done

cason · 2023-06-29T13:56:43Z

I don't know what is the best solution here.

Fixing the documentation of the p2p parameters is an issue I plan to tackle from next week, as part of #1038. This issue is more extensive, but in particular addresses the documentation of the parameter introduced by this PR.

Reverting the original PR on main and v0.38.x and replace for a correct and minimally unit-tested solution is also on my plans, with support of @glnro.

sergio-mena · 2023-06-30T17:04:45Z

Reverting the original PR on main and v0.38.x and replace for a correct and minimally unit-tested solution is also on my plans, with support of @glnro.

Yes, please, go ahead and drive it to completion. I don't think it's a lot of work to fix the doc messages on the original patch, and add a UT to test the added code.

cason · 2023-07-11T07:22:27Z

Rejected. Please refer to #1109 for more detailed reasons.

cason changed the base branch from main to v0.37.x June 23, 2023 07:00

cason linked an issue Jun 23, 2023 that may be closed by this pull request

Backport "config: add bootstrap peers" (#9680) to v0.37.x #1016

Open

4 tasks

cason self-assigned this Jun 23, 2023

cason added config p2p labels Jun 23, 2023

cason pushed a commit that referenced this pull request Jun 23, 2023

Adding changelog entry for #1019, #9680 backport

b1f263f

cason marked this pull request as ready for review June 23, 2023 07:15

cason requested review from a team as code owners June 23, 2023 07:15

cason changed the title ~~v0.37.x: backport "config: add bootstrap peers (#9680)"~~ [backport to v0.37.x] config: add bootstrap peers (tendermint#9680) Jun 23, 2023

cason pushed a commit that referenced this pull request Jun 26, 2023

Adding changelog entry for #1019, #9680 backport

e27d3c8

cason force-pushed the cason/backport-9680-v0.37 branch from b1f263f to e27d3c8 Compare June 26, 2023 13:02

jmalicevic reviewed Jun 27, 2023

View reviewed changes

hvanz approved these changes Jun 27, 2023

View reviewed changes

jmalicevic approved these changes Jun 27, 2023

View reviewed changes

sergio-mena reviewed Jun 27, 2023

View reviewed changes

thanethomson reviewed Jun 27, 2023

View reviewed changes

cason commented Jun 28, 2023

View reviewed changes

cason added the wip Work in progress label Jun 28, 2023

Daniel Cason added 2 commits June 29, 2023 15:49

config: add bootstrap peers (#9680)

57a6b43

Adding changelog entry for #1019, #9680 backport

6d70663

cason force-pushed the cason/backport-9680-v0.37 branch from e27d3c8 to 6d70663 Compare June 29, 2023 13:49

cason mentioned this pull request Jun 29, 2023

Backport "config: add bootstrap peers" (#9680) to v0.37.x #1016

Open

4 tasks

cason mentioned this pull request Jul 11, 2023

Revert "config: add bootstrap peers (#9680)" #1109

Merged

cason closed this Jul 11, 2023

zrbecker deleted the cason/backport-9680-v0.37 branch February 7, 2025 17:07

		@@ -0,0 +1,3 @@
		- `[config]` Introduce `BootstrapPeers` to the config to allow

	- `[config]` Introduce `BootstrapPeers` to the config to allow
	- `[config]` Introduce `p2p.bootstrap_peers` to the config to allow

	# on startup. Either BootstrapPeers or PersistentPeers are
	# on startup. Either bootstrap_peers or persistent_peers are

Conversation

cason commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvanz left a comment

Choose a reason for hiding this comment

Uh oh!

jmalicevic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cason commented Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cason commented Jun 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergio-mena commented Jun 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cason commented Jun 28, 2023

Uh oh!

sergio-mena commented Jun 28, 2023

Uh oh!

cason commented Jun 28, 2023

Uh oh!

sergio-mena commented Jun 29, 2023

Uh oh!

cason commented Jun 29, 2023

Uh oh!

sergio-mena commented Jun 30, 2023

Uh oh!

cason commented Jul 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cason commented Jun 23, 2023 •

edited

Loading

cason commented Jun 27, 2023 •

edited

Loading