feat: Add incentivization PoC for RLNaaS in Lightpush

[s-tikhomirov]

Description

This WIP PR implements a PoC for light protocol incentivization as outlined in:

• Incentivization spec
• Incentivized Lightpush - Vac post

Changes

• cherry-pick prior PoC work from an experimental branch
• remove the dummy protocol (not needed anymore)
• implement the required transaction checks (payment amount, address)
• avoid hard-coded parameters (in the main code, not in tests)
• track used txids server-side to avoid double spend
• integrate with Lightpush

Some items (especially those closer to the end of this list) may be moved to later PRs to keep change sets relatively small.

[github-actions]

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:3166

Built from c5f7990

[Ivansete-status]

@s-tikhomirov - is there anything blocking this PR?

[s-tikhomirov]

@s-tikhomirov - is there anything blocking this PR?

Good question.

On the one hand, there are still things to implement before this can be considered a fully-fledged PoC. For example, the properties of a transaction that is checked for "eligibility" are now simply hard-coded. In the final vision, those should be node-specific and stored in a config file or something like that (and I'd have to figure out where such files should be located, how to manage them, etc). On the other hand, the current version is more or less self-contained, and in the spirit of merging things incrementally, I could see this PR at least considered for review.

For context: the overall vision of this PoC is that a client would attach a txid as proof of payment alongside its Lightpush request, and the server would check for eligibility of that txid. By eligibility we mean that the transaction:

• has been confirmed;
• is not a contract creation transaction;
• pays the expected amount;
• pays to the expected destination;
• hasn't been used in a previous request.

Here is what is implemented now:

• check that the tx has been confirmed;
• check that the tx is a simple transfer (not a contract creation and not a contract call);
• check that the amount is "expected" (a hard-coded amount from a totally random tx used in tests);
• check that the destination is "expected" (also hard-coded, same as with the amount).

These could be the next steps:

• avoid hard-coded values (use server-specific expected amounts and destinations);
• use token transfer transactions and not simple transfers (as described here; I'm using simple transfers now for simplicity);
• account for double-spent txids;
• avoid hard-coded Infura endpoint for querying the blockchain;
• attach this to Lightpush (behind a feature flag).

Given this, do you think it makes sense to consider this PR for review as it stands now?

@jm-clius , WDYT?

[jm-clius]

Without having looked at the code itself yet, based on your description I would certainly suggest opening for review - or even selecting a smaller increment that illustrates a specific function and creating a PR for that. This way we can show the POC growing, remain clear about next steps and what is WIP, while making the turnaround from PR to merge shorter due to the easier review burden on nwaku contributors.

[gabrielmer]

Amazing work! Thanks so much! 🤩

I added some questions and comments, please let me know if I'm missing anything in any of them :)

I also see that the Lint CI job is failing, please try installing the latest version of https://github.com/arnetheduck/nph/tree/latest and integrate it with your IDE (let me know if you have any issues with it, I can help out!)

[Ivansete-status]

Thanks for it!
I just added some nitpick comment that I hope you find useful

[jm-clius]

Thanks! Added a couple of initial comments to address below.

[jm-clius]

This seems to me to be interesting only for tests? In other words, this should form part of the test files as a helper function.

[s-tikhomirov]

My idea was, on the contrary, that this function (renamed to new in later commits) would be useful for future code that would use our eligibility logic to convert a boolean eligibility value into EligibilityStatus that will be attached to the response. Something like this:

let proof = ... # extract eligibility proof (not necessarily a txid) from the request
let isValid = isEligible(proof, ...) # check proof eligibility
let eligibilityStatus = new(EligibilityStatus, isValid)
let response = LightpushResponse(eligibilityStatus, ...) # generate response
return response # (send the response back to the client)

Does it make sense?

[s-tikhomirov]

Re-requesting reviews after addressing the suggestions (one suggestion remains unresolved, but it shouldn't be a blocker).

[gabrielmer]

Oooh looks great! 🤩

I added comments on some last small details - overall looks amazing! Thanks so much!

lmk in case anything I proposed doesn't make sense :))

[gabrielmer]

Looks greatt! Thanks so much! 🤩

[Ivansete-status]

Thanks for it! I've added some nitpick comments that I hope you find useful
I'm happy to re-review later on :) Ping me if need any clarification 🥳

[Ivansete-status]

It is generally better to avoid external dependencies in tests. I suggest taking a look on how Anvil is being used in other RLN tests.

For example, tests/waku_rln_relay/test_rln_group_manager_onchain.nim and waku/waku_rln_relay/constants.nim can serve as a good reference :) ( notice EthClient* = "http://127.0.0.1:8540" is defined in constants and it deals with a local Anvil instance )

[Ivansete-status]

I think no need to be public

Suggested change

	const TxHashNonExisting* =
	TxHash.fromHex("0x0000000000000000000000000000000000000000000000000000000000000000")
	const TxHashContractCreation* =
	TxHash.fromHex("0xa2e39bee557144591fb7b2891ef44e1392f86c5ba1fc0afb6c0e862676ffd50f")
	const TxHashContractCall* =
	TxHash.fromHex("0x2761f066eeae9a259a0247f529133dd01b7f57bf74254a64d897433397d321cb")
	const TxHashSimpleTransfer* =
	TxHash.fromHex("0xa3985984b2ec3f1c3d473eb57a4820a56748f25dabbf9414f2b8380312b439cc")
	const TxHashNonExisting =
	TxHash.fromHex("0x0000000000000000000000000000000000000000000000000000000000000000")
	const TxHashContractCreation =
	TxHash.fromHex("0xa2e39bee557144591fb7b2891ef44e1392f86c5ba1fc0afb6c0e862676ffd50f")
	const TxHashContractCall =
	TxHash.fromHex("0x2761f066eeae9a259a0247f529133dd01b7f57bf74254a64d897433397d321cb")
	const TxHashSimpleTransfer =
	TxHash.fromHex("0xa3985984b2ec3f1c3d473eb57a4820a56748f25dabbf9414f2b8380312b439cc")

[Ivansete-status]

In this particular case I think is better to use the assert approach

Suggested change

	check:
	isEligible.isOk()
	assert isEligible.isOk(), isEligible.error

[Ivansete-status]

By Nim convention, the object types (stack allocated) should be created with init. The new word should be reserved for only ref objects (heap allocated.)

Suggested change

	proc new*(T: type EligibilityStatus, isEligible: bool): T =
	proc init*(T: type EligibilityStatus, isEligible: bool): T =

[Ivansete-status]

Maybe not needed chronos?

Suggested change

	import std/options, chronos
	import std/options

[Ivansete-status]

Creating a new instance of web3 for every call to isEligibleTxId doesn't sound very efficient. I believe it is better to do that once, when the app starts, and only close the web3 instance at the app's end.

I'd suggest creating a new type EligibilityManager that contains the web3 instance.

[Ivansete-status]

Nitpick comment: by convention, the "verbs" should always have parenthesis when using them

Suggested change

	if toAddressOption.isNone:
	if toAddressOption.isNone():

[Ivansete-status]

Suggested change

	if txReceipt.isErr:
	if txReceipt.isErr():

[Ivansete-status]

In this particular case we can also append the error description

Suggested change

	return err("Cannot get tx receipt")
	return err("Cannot get tx receipt: " & txReceipt.error)

[Ivansete-status]

Suggested change

	if eligibilityProof.proofOfPayment.isNone:
	if eligibilityProof.proofOfPayment.isNone():