Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transaction builder over validates #4813

Closed
SWvheerden opened this issue Oct 17, 2022 · 4 comments · Fixed by #4981
Closed

Transaction builder over validates #4813

SWvheerden opened this issue Oct 17, 2022 · 4 comments · Fixed by #4981
Assignees
Labels
A-wallet Area - related to the wallet C-performance adds no new functionality but improves performance. Benchmarks will be included. P-controversial Process - This PR or Issue is controversial and/or requires more attention that simpler issues

Comments

@SWvheerden
Copy link
Collaborator

The transaction builder seems to validate every single time we add an output, add a signature etc.
It also validates at the end when it builds.

We can either remove the final validation check, or the individual checks as it does not seem performant to validate everything every single time.

@SWvheerden SWvheerden added the C-performance adds no new functionality but improves performance. Benchmarks will be included. label Oct 17, 2022
@hansieodendaal
Copy link
Contributor

It seems worthwhile to investigate this, but I would be really cautious in analysing the validation dependency chain.

@SWvheerden
Copy link
Collaborator Author

SWvheerden commented Oct 18, 2022

The only place where a transaction really needs to be validated for chain security is on the base nodes when accepting a transaction to be put into the mempool, and when accepting a new block.

The rest are all just sanity checks.
But we are overdoing sanity checks as they are not free, they ain't expensive, but they ain't free either.

@hansieodendaal
Copy link
Contributor

hansieodendaal commented Oct 18, 2022

And from the wallet's perspective?

@SWvheerden
Copy link
Collaborator Author

Its all just sanity checks, and not even complete checks as the wallet cannot check consensus rules such as duplicate outputs etc.
All it can do is check the balance and signatures. We don't even check that the inputs are spendable.

@stringhandler stringhandler added A-wallet Area - related to the wallet P-controversial Process - This PR or Issue is controversial and/or requires more attention that simpler issues labels Oct 31, 2022
@stringhandler stringhandler added this to the Stagenet Freeze milestone Oct 31, 2022
@stringhandler stringhandler moved this to Selected for development in Tari Esme Testnet Nov 15, 2022
@agubarev agubarev self-assigned this Nov 21, 2022
@agubarev agubarev moved this from Selected for development to In Progress in Tari Esme Testnet Nov 28, 2022
@sdbondi sdbondi moved this from In Progress to Selected for development in Tari Esme Testnet Nov 29, 2022
@SWvheerden SWvheerden moved this from Selected for development to In Progress in Tari Esme Testnet Nov 29, 2022
@SWvheerden SWvheerden moved this from In Progress to Selected for development in Tari Esme Testnet Nov 29, 2022
@SWvheerden SWvheerden moved this from Selected for development to In Progress in Tari Esme Testnet Nov 29, 2022
@SWvheerden SWvheerden self-assigned this Nov 29, 2022
@stringhandler stringhandler moved this from In Progress to In Review in Tari Esme Testnet Dec 1, 2022
stringhandler pushed a commit that referenced this issue Dec 1, 2022
Description
---
This removes extra validation from transaction protocols. 

Motivation and Context
---
The transaction protocols do a lot of extra unnecessary validation. 
For example:
The code will run add_output(output), which will validate the range_proof and signatures. 
The next line will run tx.build(), which will again validate the range_proof and signatures. 
While these tests are not expensive, they are not free either. 

The second problem is that when we do aggregate transactions such as with branch: `feature-m-of-n` these validations fail as the signatures are partial until right before we build the final transaction

How Has This Been Tested?
---
Passes all unit tests

Fixes: #4813
Repository owner moved this from In Review to Done in Tari Esme Testnet Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-wallet Area - related to the wallet C-performance adds no new functionality but improves performance. Benchmarks will be included. P-controversial Process - This PR or Issue is controversial and/or requires more attention that simpler issues
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants