node: Node knows of its own Type #123

renaynay · 2021-10-08T15:09:39Z

This PR fixes a bug where a Full node would have the Light type assigned to it during creation.

Resolves #122.

liamsi

I'm not too familiar with the uber fx framework but the PR looks good to me.

liamsi · 2021-10-08T17:35:34Z

node/node.go

@@ -60,7 +60,15 @@ func New(tp Type, repo Repository) (*Node, error) {
 	case Full:
 		return newNode(fullComponents(cfg, repo))
 	case Light:
-		return newNode(lightComponents(cfg, repo))
+		// TODO @renaynay @wondertan: hacky, but fx.Provide does not allow two values to be written,


What are the 2 values?

by two values, I mean providing two different values to fx for the same field.

mattdf

Should rework this to have both fullComponents and lightComponents call a baseComponents func rather than hacky patch the light case, but if it works good for now.

Wondertan · 2021-10-09T09:36:33Z

There is a solution for this hidden in #97(b739077). It is a bit cleaner, but I also like this one.

renaynay · 2021-10-11T08:53:24Z

Closing in favor of solution present in #97 , thank you @Wondertan

--------- Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com>

Disable "fail-fast" so if the `ubuntu-latest` matrix unit test fails then it wont cancel the run of `macos-latest`, this will help with easing ability to merge as we've reduced the requirement for both to merge

…#2913)  Added these in the run up to cutting 0.12.0 so hopefully the tag/release pipeline would generate signed binaries and not halt on any of these three flakey test runs. Setting these back to fail explicitly so we can see them go red and feel bad, and now start to fix them

Fixes #2936 This PR updates the `Start` command to accept a variadic number of `funcs` which can be used to customise the command. This allows the caller to hook into and modify the command, for example to add a `PreRun` hook, as required for `celestia-da`. With this PR in place, the `celestia-da` requirement, which needs to add some required flags and add a `PreRun` hook can be satisfied as: ```go bridgeCmd := bridge.NewCommand(WithPreRun()) lightCmd := light.NewCommand(WithPreRun()) fullCmd := full.NewCommand(WithPreRun()) ``` Ideally this can be applied for other commands `Init`, `Auth` as well, but it's not required for the current requirement.

Fixes #3003.  --------- Co-authored-by: ramin <raminkeene@gmail.com>

Signed-off-by: Smuu <18609909+Smuu@users.noreply.github.com> Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>

Renames `swamp` to `integration` in Makefile and where called in github actions workflows Fixes #2104

reduced myself to fixing dependabot's attempts to upgrade us at #3055 was not working quite right

in kind replacement of #2998 with the simplified require assertion --------- Co-authored-by: Håvard Anda Estensen <haavard.ae@gmail.com>

Renames `swamp` to `integration` in Makefile and where called in github actions workflows Fixes #2104

reduced myself to fixing dependabot's attempts to upgrade us at #3055 was not working quite right

in kind replacement of #2998 with the simplified require assertion --------- Co-authored-by: Håvard Anda Estensen <haavard.ae@gmail.com>

Adding `continue-on-error` to the flakey components so we can see that the entire pipeline w/ generated binaries will complete ASAP / next release, will be replaced with separating/fixing the flakey tests next.

Renames `swamp` to `integration` in Makefile and where called in github actions workflows Fixes #2104

reduced myself to fixing dependabot's attempts to upgrade us at #3055 was not working quite right

in kind replacement of #2998 with the simplified require assertion --------- Co-authored-by: Håvard Anda Estensen <haavard.ae@gmail.com>

Adding `continue-on-error` to the flakey components so we can see that the entire pipeline w/ generated binaries will complete ASAP / next release, will be replaced with separating/fixing the flakey tests next.

Closes #2382 --- ## Screenshot ![image](https://github.com/celestiaorg/celestia-node/assets/25278203/2c04a166-0d41-45df-8194-20cc51ef1ba1) --------- Co-authored-by: ramin <raminkeene@gmail.com>

…ons (#3072)  refactor: changes SubmitTX permissions from write to read. Following the issue requirement, this PR changes SubmitTX permissions from _write_ to _read_ Changes: * Updated nodebuilder/state/state.go from **write** to **read** permissions * Updated following tests (api/rpc_test.go) and added extra check for SubmitTX rpc call (read permissions check). closes #2958 --------- Co-authored-by: [NODERS]TEAM <office@noders.team> Co-authored-by: ramin <raminkeene@gmail.com>

Closes #2382 --- ## Screenshot ![image](https://github.com/celestiaorg/celestia-node/assets/25278203/2c04a166-0d41-45df-8194-20cc51ef1ba1) --------- Co-authored-by: ramin <raminkeene@gmail.com>

…ons (#3072)  refactor: changes SubmitTX permissions from write to read. Following the issue requirement, this PR changes SubmitTX permissions from _write_ to _read_ Changes: * Updated nodebuilder/state/state.go from **write** to **read** permissions * Updated following tests (api/rpc_test.go) and added extra check for SubmitTX rpc call (read permissions check). closes #2958 --------- Co-authored-by: [NODERS]TEAM <office@noders.team> Co-authored-by: ramin <raminkeene@gmail.com>

@vgonkivs

Building off @vgonkivs's [work to skip tests](#2802), this attempts to get a baseline "green" ci for us. There are still a couple of tests that VERY intermittently flake in CI, but this way they will stand out WHEN they happen and we can track down vs entire thing being red always. This does quite a few things - introduces build tags on swamp tests in each named file to allow us to run parts of the swamp/integration tests independently (ie: `go test ./... -tags=blob` - adds an `integration` tag to allow running all still - splits the integration tests into it's own workflow file (`integration-tests.yml`) which is now triggered from `go-ci.yml` - splits each swamp/integration tests to run as its own job so we can see which are failing/flakey more explicitly - utilizes go's -short test flag to Skip a few swamp/integration tests that are consistently failing. Current we are skipping `TestFullReconstructFromFulls`, `TestFullReconstructFromLights`, `TestSyncStartStopLightWithBridge` which is less than we were originally skipping in #2802 - plugs in our verbose/debug stuff to integration tests in addition to unit which we had before Unit tests - splits some of the unit tests that were "race flakey" into `*_no_race_test.go` and adds `!race` tag to some others to get to pass consistently when running unit tests with -race flag - macos-latest unit tests still fail on race test ONLY in GitHub actions CI 🤷‍♂️ Next Steps - create issues for each short run / skipped - create issues for any tests that fail race that are NOT the race issue on upstream cosmos-sdk - create issue for macos-latest in GitHub race fail - create issue for macos-latest intermittent fail I think we let this run for a while, then once we see it be consistently green for a bit and isolate any more flakes that pop up, we can toggle on more branch requirements, then in fixing the above issues, we can be green. [Being green is not easy](https://www.youtube.com/watch?v=51BQfPeSK8k).

Used a tool to find stray spellings then fix them, hopefully this will prevent future small PRs for little fixes emerging and we can encourage more significant / valuable PRs from external contributors

suggesting we YEET the `unit test with race` temporarily as the job intermittently fails across a handful of tests pretty regularly and we don't require this job to pass for a PR merge anyway. Lets remove, i'll get it working and also enable for `macos-latest` which fails ALWAYS on github then we can / replace the normal unit tests with unit tests with race after we get it all running and keep CI green vs just ignoring a red x most of the time

# Summary Open RPC spec generation errors, due to a missing sample value for blob.GasPrice. # Problem Missing sample value in `api/docgen/examples.go`. Logs from running on v0.13.0: ```json --> Generating OpenRPC spec failed to retrieve example value for type: blob.GasPrice on parent 'blob.GasPrice') { "openrpc": "1.2.6", "info": { "title": "Celestia Node API", "description": "The Celestia Node API is the collection of RPC methods that can be used to interact with the services provided by Celestia Data Availability Nodes.", "version": "v0.11.0" }, ``` Output from logs running on v0.11.0, as another check to see that this worked before: ```json --> Generating OpenRPC spec { "openrpc": "1.2.6", "info": { "title": "Celestia Node API", "description": "The Celestia Node API is the collection of RPC methods that can be used to interact with the services provided by Celestia Data Availability Nodes.", "version": "v0.11.0" }, ``` # Solution Add sample value. [Output after testing](https://gist.github.com/jcstein/02c459fb781889405d7ebc802484509a) ## Other issues found while testing API version is still v0.11.0, which is the same as last few releases, i just ran make openrpc-gen on v0.11.0 and the API version is the same there. This is already, addressed in #2549, and I am highlighting to bring it out from stale state. ### Proposed solutions for other issues found To avoid this in the future, i would like to see if we can work this [feature request to add openrpc.json on main](#2611) into celestia-node, so that there is validation before publishing specs, and a history of past versions of the openrpc.json, stored on main with releases. otherwise we JTMB the person updating node-rpc-docs [on almost 10000 lines of spec code](https://github.com/celestiaorg/node-rpc-docs/pull/35/files) > Note, this output must not contain the output which precedes the json: > `--> Generating OpenRPC spec`

These are now [generally available](https://github.blog/changelog/2024-01-30-github-actions-introducing-the-new-m1-macos-runner-available-to-open-source/) and these will a) be faster b) also be closer to developer machines also removed a commented out job in the ci_release.yml file that had been sitting dormant for a few years

#3172)  - split the new flakey DA tests into own tag and call so we don't think blob has started failing suddenly - Total shutdown of DA integration test `GetIDs` until we can figure out what is going on

…#3227)  Updates the CI workflow trigger to run specifically from a release publish, vs pushing a tag. This will separate any testing we do with with any pre-release or temporary tags from triggering the GitHub workflow to generate a release. Instead, only the release published event should trigger this. --------- Co-authored-by: Matthew Sevey <mjsevey@gmail.com>

@rootulp

…nstall everywhere else (#3197)  refs #3169 Suggestion we add this little detect and switch. One issue i see with attempting to detect GOBIN is that it doesn't HAVE to be set (ie: there can be a default). This i would think is a good quick compromise to un-confuse @rootulp and (potentially) other devs and make this behavior more delightful most of the time. IF not, we should close this and the other issue. --------- Co-authored-by: Rootul P <rootulp@gmail.com>

…erged (#3247)  with our `1.22` upgrade i got inadvertently upgraded to `golangci-lint` 1.56 locally and experiencing different results to what was in CI so figured, why not upgrade, i believe this upgrade makes 'revive' active and a replacement to golint, which introduced some better/stricter checks - upgrades `golangci-lint` to 1.56 - fixes or marks `//nolint:revive` a few new unused variable errors that were triggering - configures revive to ignore `_test` files - CI passes with new checks

Bump fixes variable name for slack notifications.

Error 404: https://docs.tendermint.com/v0.35/nodes/metrics.html Fix: https://docs.tendermint.com/main/tendermint-core/metrics.html  --------- Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>

#3351)  I realized that the `published` event is triggered by pre-releases too. The `released` event is only triggered when we set the release to the latest release, which is what we want to track for deployments.

Error 404: https://docs.tendermint.com/v0.35/nodes/metrics.html Fix: https://docs.tendermint.com/main/tendermint-core/metrics.html  --------- Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com> (cherry picked from commit b6a77a6)

celestiaorg#3351)  I realized that the `published` event is triggered by pre-releases too. The `released` event is only triggered when we set the release to the latest release, which is what we want to track for deployments. (cherry picked from commit 40ee3c5)

Error 404: https://docs.tendermint.com/v0.35/nodes/metrics.html Fix: https://docs.tendermint.com/main/tendermint-core/metrics.html  --------- Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>

celestiaorg#3351)  I realized that the `published` event is triggered by pre-releases too. The `released` event is only triggered when we set the release to the latest release, which is what we want to track for deployments.

use https://github.com/Abirdcfly/dupword to check duplicate words output: ``` celestia-node/share/eds/retriever_quadrant.go:71:4: Duplicate words (into) found celestia-node/share/availability/full/reconstruction_test.go:179:2: Duplicate words (L) found celestia-node/share/getters/getter_test.go:338:2: Duplicate words (D) found celestia-node/share/p2p/peers/manager_test.go:86:3: Duplicate words (be) found ```

Opening as a replacement for #2956 which has gone stale. I had added some tests to the original refactoring but original author vanished without allowing admin's to modify the original PR/branch. From original note: fixes: #2932 , #2908 --------- Co-authored-by: Atreay Kukanur <66585295+ATREAY@users.noreply.github.com>

fix function names in comment Signed-off-by: studystill <chenghuiyue@outlook.com>

…#3350)  Adds a step in the ci_and_release pipeline to generate `openrpc.json` using `make openrpc-gen` and store that generated file with the release artifacts. This is set to run after go-releaser, though we could have made this its own workflow file potentially. I have tested that release upload works in a personal repo, but as with all things on github, we can only REALLY test and do this one live 🤞 refs #2611

@node

- Expand individual members of CODEOWNERS - (we should consider there being a @node GitHub team for this eventually)

Self-explanatory  --------- Co-authored-by: ramin <raminkeene@gmail.com>

node knows of its own type

916223e

renaynay added the area:node Node label Oct 8, 2021

renaynay requested review from liamsi, mattdf and Wondertan as code owners October 8, 2021 15:09

renaynay added this to In Review in Devnet [ ARCHIVED ] Oct 8, 2021

liamsi approved these changes Oct 8, 2021

View reviewed changes

liamsi reviewed Oct 8, 2021

View reviewed changes

mattdf approved these changes Oct 8, 2021

View reviewed changes

renaynay closed this Oct 11, 2021

liamsi mentioned this pull request Oct 11, 2021

node/rpc: Implement simple RPC server #124

Merged

vgonkivs mentioned this pull request Jun 28, 2024

fix(cmd): fix namespace representation in cmd output #3537

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node: Node knows of its own Type #123

node: Node knows of its own Type #123

renaynay commented Oct 8, 2021

liamsi left a comment

liamsi Oct 8, 2021

renaynay Oct 8, 2021

mattdf left a comment

Wondertan commented Oct 9, 2021

renaynay commented Oct 11, 2021

node: Node knows of its own Type #123

node: Node knows of its own Type #123

Conversation

renaynay commented Oct 8, 2021

liamsi left a comment

Choose a reason for hiding this comment

liamsi Oct 8, 2021

Choose a reason for hiding this comment

renaynay Oct 8, 2021

Choose a reason for hiding this comment

mattdf left a comment

Choose a reason for hiding this comment

Wondertan commented Oct 9, 2021

renaynay commented Oct 11, 2021