[Miner] feat: add `Miner` component #168

bryanchriswhite · 2023-11-09T13:25:31Z

Summary

Human Summary

Ports and refactors the Miner component from poktroll-alpha.

AI Summary

Summary generated by Reviewpad on 10 Nov 23 20:30 UTC

This pull request includes the following changes:

The interface.go file in the pkg/relayer package has been modified to add a new import statement and several interface definitions and updates. These changes are related to the implementation of the relayer's session lifecycles and the introduction of a new miner interface.
A new file named "miner.go" has been added in the "pkg/relayer/miner" directory. This file defines a package called "miner" and includes the implementation of the relayer.Miner interface. It also contains some utility functions and a constructor for creating a new miner.
The relayerSessionsManager struct, defined in a separate file, has been modified to include a new field, relayObs, of type observable.Observable[*relayer.MinedRelay]. The struct also includes methods for managing the relayer's session lifecycles, such as starting and stopping sessions, inserting relays, and ensuring the session tree.
Another file named "claim.go" has been added in the pkg/relayer/session package. This file defines the relayerSessionsManager struct and includes methods for creating claims for relayer sessions. These methods use observables to handle asynchronous operations and error handling.
A new file named "miner_test.go" has been added in the pkg/relayer/miner package. This file includes a single test function that is currently skipped, indicating that more test coverage needs to be added for the miner implementation.
The types.go file in the pkg/relayer package has been modified to introduce a new struct called MinedRelay, which is a wrapper around a serialized and hashed relay. This struct contains two fields, Bytes and Hash, which are of type []byte.
The types.go file in the pkg/either package has been modified to add a new import statement and update the SessionTree type, which is now an Either type wrapping relayer.SessionTree. Some type aliases have also been reformatted for clarity.
A new file named "difficulty.go" has been added in the pkg/relayer/protocol directory. This file includes the implementation of a function that checks if a given byte slice exceeds a specified difficulty. This function is used to determine if a relay is volume applicable.

These changes are related to the implementation of the relayer's session lifecycles, the introduction of a new miner interface, and the handling of relay claims and proofs. Some areas, such as test coverage and algorithm optimization, have been identified for future improvements.

Please let me know if you need any further assistance!

Issue

[Miner] Foundation for the Miner submodule #13

Adds the miner component.

Type of change

Select one or more:

Testing

Run all unit tests: make go_develop_and_test
Verify Localnet manually: See the instructions [here](TODO: add link to instructions)

Sanity Checklist

I have tested my changes using the available tooling
I have performed a self-review of my own code
I have commented my code, updated documentation and left TODOs throughout the codebase

red-0ne

servicetypes.Relay is a struct, we should work with its pointer

pkg/relayer/interface.go

pkg/relayer/miner/miner.go

Olshansk

@bryanchriswhite NOT a complete review, but just a preliminary one to give early feedback.

pkg/relayer/interface.go

pkg/relayer/protocol/difficulty.go

pkg/relayer/miner/miner.go

pkg/relayer/miner/miner_test.go

pkg/relayer/protocol/block_heights.go

* pokt/main: [Off-chain] feat: observable utils (#171) [Off-chain] refactor: `MapFn`s receive context arg (#170)

- Fixed helpers for localnet regenesis - Added an application & supplier to the genesis file - Initializing appMap & supplierMap in E2E tests - Add support for the app's codec (for unmarshaling responses) in E2E tests - Adding a placeholder for `e2e/tests/relay.feature` --- Co-authored-by: harry <[email protected]>

* refactor: `MapFn`s receive context arg * feat: add `MapExpand` observable operator * refactor: `RelayerSessionsManager` to be more reactive * chore: add godoc comment * chore: review feedback improvements * trigger CI

pkg/relayer/miner/miner.go

Olshansk · 2023-11-09T23:06:27Z

pkg/relayer/miner/miner.go

+	earliestCreateClaimHeight :=
+		protocol.GetEarliestCreateClaimHeight(createClaimWindowStartBlock)
+
+	log.Printf("earliest claim submission createClaimWindowStartBlock height for this supplier: %d", earliestCreateClaimHeight)


So are we going to have a bunch of blocking calls to this at any point in time?

There will be blocking calls made in map functions to delay downstream notifications to happen at precise times.

pkg/relayer/miner/miner.go

* pokt/main: [Proxy] chore: Use depinject for relayerProxy (#173) [Sessions] chore: Use depinject for sessions mgr construction (#175)

…ger`.

pkg/relayer/miner/miner.go

pkg/relayer/cmd/cmd.go

Olshansk · 2023-11-10T18:43:02Z

pkg/relayer/interface.go

+	MinedRelays(
+		ctx context.Context,
+		servedRelayObs observable.Observable[*servicetypes.Relay],
+	) (minedRelaysObs observable.Observable[*MinedRelay])


UNRELATED OPTION NIT: Thoughts on adding a TODO_TECHDEBT: Add the Obs suffix to all Observables in the codebase.

Might be worthing putting it in observable.go if you also agree that it's the right pattern to follow.

Olshansk · 2023-11-10T18:46:52Z

pkg/relayer/interface.go

-	EnsureSessionTree(sessionHeader *sessiontypes.SessionHeader) (SessionTree, error)
+	// IncludeRelays receives an observable of relays that should be included
+	// in their respective session's SMST (tree).
+	IncludeRelays(relayObs observable.Observable[*MinedRelay])


Thoughts on this rename?

Suggested change

IncludeRelays(relayObs observable.Observable[*MinedRelay])

ClaimRelays(minedRelaysObs observable.Observable[*MinedRelay])

My hesitation with ClaimRelays is that while claming is a consequence of having relays in the SMST by a certain time, that's something that happens as an indirect consequence of IncludeRelays. What this method does directly is insert the observed relays in to the respective session's SMST. Start is where the observable mapping happens such that claims and proofs are created/submitted.

What about InsertRelays?

Discussed offline. Proof of digital handshake below:

Olshansk · 2023-11-10T18:49:04Z

pkg/relayer/interface.go

+	// network as necessary.
+	Start(ctx context.Context)
+
+	// Stop unsubscribes all observables from the RelaysToInclude observable which


I couldn't find RelaysToInclude so should this be minedRelaysObs if we rename the parameter above appropriatelY?

Nice catch, thanks! The method was renamed to IncludeRelays. It's defined on the RelayerSessionManger interface and receives minedRelaysObs. I intentionally avoided referencing mining on that side of the RelayerSessionManager interface because it doesn't need to know anything about mining other than the fact that the MinedRelay provides bytes and hash fields (which could be replaced with and interface in future refactoring).

pkg/relayer/miner/miner.go

pkg/relayer/session/claim.go

pkg/relayer/session/session.go

Olshansk · 2023-11-10T19:41:31Z

@bryanchriswhite All of my comments are only related to naming, comment updates, etc...

Overall, great work and thank you for iterating on top of yesterday's feedback. I feel like this is MUCH MUCH easier to read and understand!

Co-authored-by: Daniel Olshansky <[email protected]>

red-0ne

LGTM! Splitting responsibilities this way makes it way easier to reason about.

I'll base RelayMiner on this work.

Olshansk

* pokt/relayer/cli: [AppGate] Implement the MVP AppGateServer (#108) chore: Improve comment about startig relayer proxy [Relayer] feat: Add Relayer struct (#172) feat: Use relay miner to start chore: Reflect responsibility changes of session manager [Miner] feat: add `Miner` component (#168) fix: Update Miner interface chore: update start mining comment chore: Remove unused RelayerOption parameter chore: Rename relay miner file chore: Rename to RelayMiner feat: Add Relayer struct

* refactor: `MapFn`s receive context arg * chore: add `ForEach` map shorthand operator * chore: add `/pkg/observable/filter` * chore: add `/pkg/observable/logging` * chore: add `/pkg/relayer/protocol` * chore: add `Miner` interface * feat: add `Miner` implementation * test: `Miner` implementation * chore: fix comment * chore: add godoc comments * [Test] First step for automated E2E Relay test (#167) - Fixed helpers for localnet regenesis - Added an application & supplier to the genesis file - Initializing appMap & supplierMap in E2E tests - Add support for the app's codec (for unmarshaling responses) in E2E tests - Adding a placeholder for `e2e/tests/relay.feature` --- Co-authored-by: harry <[email protected]> * [Relayer] refactor: simplify `RelayerSessionsManager` (#169) * refactor: `MapFn`s receive context arg * feat: add `MapExpand` observable operator * refactor: `RelayerSessionsManager` to be more reactive * chore: add godoc comment * chore: review feedback improvements * trigger CI * chore: review feedback improvements Co-authored-by: Daniel Olshansky <[email protected]> * chore: review feedback improvements * fix: import cycle & goimports * chore: review feedback improvements * chore: cleanup TODO_THIS_COMMIT comments * chore: improve var & func names for clarity and consistency * refactor: move claim/proof lifecycle concerns to `relayerSessionsManager`. * chore: review feedback improvements * chore: review feedback improvements * refactor: `miner#hash()` method * chore: tidy up * chore: simplify * chore: review feedback improvements Co-authored-by: Daniel Olshansky <[email protected]> * chore: review feedback improvements Co-authored-by: Daniel Olshansky <[email protected]> * chore: review feedback improvements Co-authored-by: Daniel Olshansky <[email protected]> * chore: review feedback improvements * chore: review feedback improvements * fix: incomplete refactor * chore: simplify --------- Co-authored-by: Daniel Olshansky <[email protected]> Co-authored-by: harry <[email protected]>

bryanchriswhite added miner Changes related to the Miner off-chain Off-chain business logic labels Nov 9, 2023

bryanchriswhite added this to the Shannon TestNet milestone Nov 9, 2023

bryanchriswhite self-assigned this Nov 9, 2023

bryanchriswhite force-pushed the issues/13/feat/miner branch from 0f4cf29 to e91ff82 Compare November 9, 2023 13:29

refactor: MapFns receive context arg

29b9bdf

bryanchriswhite mentioned this pull request Nov 9, 2023

[Off-chain] refactor: MapFns receive context arg #170

Merged

10 tasks

bryanchriswhite force-pushed the issues/13/feat/miner branch from e91ff82 to 175f220 Compare November 9, 2023 13:55

bryanchriswhite changed the base branch from main to issues/13/refactor/map November 9, 2023 13:56

bryanchriswhite added 7 commits November 9, 2023 16:26

chore: add ForEach map shorthand operator

b6c9c71

chore: add /pkg/observable/filter

f257b46

chore: add /pkg/observable/logging

194cee5

chore: add /pkg/relayer/protocol

cca55c2

chore: add Miner interface

c666542

feat: add Miner implementation

ea8d848

test: Miner implementation

38329bb

bryanchriswhite force-pushed the issues/13/feat/miner branch from 175f220 to 38329bb Compare November 9, 2023 15:34

bryanchriswhite changed the base branch from issues/13/refactor/map to issues/13/feat/observable-utils November 9, 2023 15:35

bryanchriswhite added 2 commits November 9, 2023 16:47

chore: fix comment

d2f9cb4

chore: add godoc comments

d502b75

bryanchriswhite requested review from Olshansk and red-0ne November 9, 2023 16:41

bryanchriswhite linked an issue Nov 9, 2023 that may be closed by this pull request

[Miner] Foundation for the Miner submodule #13

Closed

10 tasks

red-0ne reviewed Nov 9, 2023

View reviewed changes

red-0ne requested changes Nov 9, 2023

View reviewed changes

pkg/relayer/miner/miner.go Outdated Show resolved Hide resolved

pkg/relayer/miner/miner.go Outdated Show resolved Hide resolved

Olshansk requested changes Nov 9, 2023

View reviewed changes

Base automatically changed from issues/13/feat/observable-utils to main November 9, 2023 20:14

bryanchriswhite and others added 3 commits November 9, 2023 21:16

Merge remote-tracking branch 'pokt/main' into issues/13/feat/miner

f85a381

* pokt/main: [Off-chain] feat: observable utils (#171) [Off-chain] refactor: `MapFn`s receive context arg (#170)

[Relayer] refactor: simplify RelayerSessionsManager (#169)

0e72490

* refactor: `MapFn`s receive context arg * feat: add `MapExpand` observable operator * refactor: `RelayerSessionsManager` to be more reactive * chore: add godoc comment * chore: review feedback improvements * trigger CI

Olshansk requested changes Nov 9, 2023

View reviewed changes

bryanchriswhite added 3 commits November 10, 2023 11:00

chore: improve var & func names for clarity and consistency

9c75b2c

Merge remote-tracking branch 'pokt/main' into issues/13/feat/miner

286e1b2

* pokt/main: [Proxy] chore: Use depinject for relayerProxy (#173) [Sessions] chore: Use depinject for sessions mgr construction (#175)

refactor: move claim/proof lifecycle concerns to `relayerSessionsMana…

a0ffe9d

…ger`.

github-actions bot reviewed Nov 10, 2023

View reviewed changes

pkg/relayer/miner/miner.go Outdated Show resolved Hide resolved

bryanchriswhite added 5 commits November 10, 2023 14:08

chore: review feedback improvements

2019637

chore: review feedback improvements

709f661

refactor: miner#hash() method

394575b

chore: tidy up

1eae9d2

chore: simplify

efb8a4e

bryanchriswhite requested review from red-0ne and Olshansk November 10, 2023 13:33

github-actions bot reviewed Nov 10, 2023

View reviewed changes

pkg/relayer/cmd/cmd.go Outdated Show resolved Hide resolved

pkg/relayer/cmd/cmd.go Outdated Show resolved Hide resolved

bryanchriswhite force-pushed the issues/13/feat/miner branch from b38aa81 to efb8a4e Compare November 10, 2023 14:23

Olshansk requested changes Nov 10, 2023

View reviewed changes

bryanchriswhite and others added 7 commits November 10, 2023 21:03

chore: review feedback improvements

3b2022a

Co-authored-by: Daniel Olshansky <[email protected]>

chore: review feedback improvements

967dddc

Co-authored-by: Daniel Olshansky <[email protected]>

chore: review feedback improvements

f9a6fb2

Co-authored-by: Daniel Olshansky <[email protected]>

chore: review feedback improvements

7d23bd4

chore: review feedback improvements

916c0dd

fix: incomplete refactor

55c8118

chore: simplify

c1784f5

bryanchriswhite requested a review from Olshansk November 10, 2023 20:52

red-0ne approved these changes Nov 10, 2023

View reviewed changes

Olshansk approved these changes Nov 10, 2023

View reviewed changes

bryanchriswhite merged commit 5af2ae9 into main Nov 10, 2023
7 checks passed

bryanchriswhite deleted the issues/13/feat/miner branch November 20, 2023 07:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Miner] feat: add `Miner` component #168

[Miner] feat: add `Miner` component #168

bryanchriswhite commented Nov 9, 2023 •

edited by reviewpad bot

Loading

red-0ne left a comment

Olshansk left a comment

Olshansk Nov 9, 2023

bryanchriswhite Nov 10, 2023 •

edited

Loading

Olshansk Nov 10, 2023

Olshansk Nov 10, 2023

bryanchriswhite Nov 10, 2023 •

edited

Loading

Olshansk Nov 10, 2023

Olshansk Nov 10, 2023

bryanchriswhite Nov 10, 2023

Olshansk commented Nov 10, 2023

red-0ne left a comment

Olshansk left a comment

	IncludeRelays(relayObs observable.Observable[*MinedRelay])
	ClaimRelays(minedRelaysObs observable.Observable[*MinedRelay])

[Miner] feat: add Miner component #168

[Miner] feat: add Miner component #168

Conversation

bryanchriswhite commented Nov 9, 2023 • edited by reviewpad bot Loading

Summary

Human Summary

AI Summary

Summary generated by Reviewpad on 10 Nov 23 20:30 UTC

Issue

Type of change

Testing

Sanity Checklist

red-0ne left a comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Olshansk Nov 9, 2023

Choose a reason for hiding this comment

bryanchriswhite Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Olshansk Nov 10, 2023

Choose a reason for hiding this comment

Olshansk Nov 10, 2023

Choose a reason for hiding this comment

bryanchriswhite Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Olshansk Nov 10, 2023

Choose a reason for hiding this comment

Olshansk Nov 10, 2023

Choose a reason for hiding this comment

bryanchriswhite Nov 10, 2023

Choose a reason for hiding this comment

Olshansk commented Nov 10, 2023

red-0ne left a comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

[Miner] feat: add `Miner` component #168

[Miner] feat: add `Miner` component #168

bryanchriswhite commented Nov 9, 2023 •

edited by reviewpad bot

Loading

bryanchriswhite Nov 10, 2023 •

edited

Loading

bryanchriswhite Nov 10, 2023 •

edited

Loading