[Core] Tx Fee Business Logic - Improve State Visibility

[jessicadaugherty]

Objective

Validate and expose fees throughout the blockchain state when a transaction (successful or not) is sent.

Origin Document

The flow shown in the diagram below, is an example where funds where sent from one account to another and fees were transmitted to the associated pool from the sender's account balance

We can validate the status of transactions and fees using the logging module.

Goals

• Verify that state-related changes related to fees and balances are correct when a transaction is sent

Deliverable

• Define and add tooling to increase visibility into the state of the database (balances of accounts and fees)
• Documentation on how LocalNet should be used to manually test an end-to-end flow, validating balances & fees while doing so

Non-goals / Non-deliverables

• Adding new business logic into utility
• Adding tx to mempool

General issue deliverables

• Update the appropriate CHANGELOG
• Update any relevant READMEs (local and/or global)

Testing Methodology

• All tests: make test_all
• LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md

---

Creator: @jessicadaugherty
Co-owner: @Olshansk

[Olshansk]

@jessicadaugherty Updated the description.

[jessicadaugherty]

Is there a reason these things can't be covered in a separate ticket?

Optional: if implementation is incomplete, a PR that finalizes tx fee management
A PR that adds additional unit tests for accounts/pool balances & fees when a tx is successful
A PR that adds additional unit tests for accounts/pool balances & fees when a tx is no successful

I think this is adding confusion and a distraction from the goal of sending a "happy" tx before designing the "unhappy" tx path.

[Olshansk]

Is there a reason these things can't be covered in a separate ticket?

Optional: if implementation is incomplete, a PR that finalizes tx fee management
A PR that adds additional unit tests for accounts/pool balances & fees when a tx is successful
A PR that adds additional unit tests for accounts/pool balances & fees when a tx is no successful

I think this is adding confusion and a distraction from the goal of sending a "happy" tx before designing the "unhappy" tx path.

Make sense. Let's add those deliverables to a separate ticket.

[Olshansk]

The scope of this ticket is going to be to take the last few commits made to the iteration 3 demo (found at https://github.com/pokt-network/pocket/tree/iteration3_demo) and productionize them.

[jessicadaugherty]

Design Considerations

In V0 today, invalid txs take fees. However, invalid txs, including those without enough POKT to cover fees, are still stored on-chain per Tendermint which can result in block bloat.

V0 plans on addressing this issue in pokt-network/pocket-core#1505

Pending confirmation from @oten91, I believe the intention is to take fees and store invalid txs in memory rather than in the block, and to block invalid txs that do not have enough fees to cover the tx).

[jessicadaugherty]

New ticket: Productionize E2E tx in mainline. Comes before this validate ticket.

[Olshansk]

New ticket: Productionize E2E tx in mainline. Comes before this validate ticket.

#364

[jessicadaugherty]

Hey @Olshansk! I've moved this into rescope for now. I would like to confirm a couple of things based on our comments above:

1. Leave this ticket as "validate txs". The goal is to gain more visibility into fees/funds in the state pre and post sending an E2E tx. If that is correct, can we better define "add tooling" - can you specify if this is an enhancement to the CLI or if this goal has changed in light of logging being applied?

2. Investigate the existing tx fee design as it is implemented in the utility module portion of the codebase now. It seems in utility/transaction.go there is logic to cover both checking accounts for balances/fees and returning errors/rejecting txs that do not have sufficient funds to cover fees.

References

fee, err := u.GetFee(msg, msg.GetActorType())
	if err != nil {
		return nil, "", err
	}
	pubKey, er := crypto.NewPublicKeyFromBytes(tx.Signature.PublicKey)
	if er != nil {
		return nil, "", typesUtil.ErrNewPublicKeyFromBytes(er)
	}
	accountAmount.Sub(accountAmount, fee)
	if accountAmount.Sign() == -1 {
		return nil, "", typesUtil.ErrInsufficientAmount(address.String())

Based on this snippet

// Does the tx pass basic validation?
	if err := transaction.ValidateBasic(); err != nil {
		return err
	}

	// Store the tx in the mempool
	return u.Mempool.AddTransaction(txProtoBytes)

It seems that we take the fee, run a validation against the signature, and if it is found to be an invalid tx, neither the tx or the fee are added to pools. If this code takes the fee but does not store it in mem/fee pool, what happens to the fee...? Aren't fees supposed to be 99% burned and 1% goes to the BlockProposer? Unless we are changing a design there as well?

Can you confirm that 1) this understanding of the code is correct and, if so, 2) if we want invalid txs and fees added to mem/fee pools but not in the block as was discussed as a V0 design? If so, then...

3. Create a second ticket to update transaction.go to update invalid fee behavior from section 2

3a. Was there any desire/intent to change the tx fee? Or make different fees for different tx types? Feels like a post V1 launch wishlist question but I wanted to check because in the Utility spec (3.7 State Changes) there is reference to a "a dynamic fee" - but today the fee is always 10,000 uPOKT and there isn't any additional information I can find about this. Anyway, would be cool to know if additional functionality is required, whether what is discussed here or otherwise to determine if a ticket around updating the actual business logic is required.

3b. Additionally, there is a comment in the fee logic of transaction.go that says:

CLEANUP: Exposed for testing purposes only

This can refer to a lot of things. Do you have any past insights from Andrew that might provide more context to future enhancements? Some ideas:

• Abstracting fees away from other transaction functions like validating signatures in the event we need to update fees
• If we decide that invalid txs don't take fees and are "nil" we will need to update the logic to check if fees are applied for the type of tx first (right now it just subtracts from account balance)
• Better error structs

Can you let me know if there is a third ticket that should be created around a general transactions cleanup (if there is more scope than what is discussed above in section 2)?

TIA for bearing with me through this comment!

[Olshansk]

Discussed the context above offline last week, so just wanted to capture a tl;dr of the discussion

At a high level, we'll need to create several tickets of work before we can dive into the pocket utility design & implementation):

• 1. Utility - Improve visibility into utility state (i.e. w/ tooling or documentation)
• 2. Utility - Document tx fee logic (valid, invalid, included in block or not)
• 3. Utility - Cleanup & refactor existing code (both the scope and changes are included here)
• 4. Utility - Define testing edge cases for PoS network (to make sure there are no regressions)
• 5.Utility - Add testing for E2E scenario (potentially using cucumber / gherkin)

This ticket will focus on (1).

[Olshansk]

The WIP for this is in the `issues/325/tx_fee_business_logic_visibility.

Neo4j is being used to visualize the fee pools (e.g. DAO), accounts (e.g. Address) and other actors (e.g. Validators) via a visual interface.