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

Renaming two variables related to Consensus Reverification of txs #1162

Closed
wants to merge 2 commits into from

Conversation

vncoelho
Copy link
Member

@vncoelho vncoelho commented Oct 18, 2019

closes #1148

Sometimes the Verify is used in another context, such as by Consensus Service, which sends a set (dictionary) of transactions that the Speaker is proposing.

In this sense, not always the node mempool is used for Verifying/Reverifying.

Thus, this renaming tries to clarify this.

@vncoelho vncoelho changed the title Refactoring Transaction Verify and Reverify mempool param to Renaming Transaction Verify and Reverify mempool param to Oct 18, 2019
@shargon
Copy link
Member

shargon commented Oct 18, 2019

This don't close #1148 ...

@vncoelho
Copy link
Member Author

vncoelho commented Oct 18, 2019

@shargon, do you still believe that happens? I think the code cover that, this renaming is just for improving comprehension.

Exactly the Verify and the possibility of sending specific sets of transactions solves that, consensus service is isolated from mempool in such case.

@vncoelho vncoelho changed the title Renaming Transaction Verify and Reverify mempool param to Renaming Transaction Verify and Reverify mempool param to transactionsPool Oct 18, 2019
* Adding random hashes for OnGetDataMessageReceived

* Adding static readonly Random

* Renaming consensus RestartTasks to ConsensusTxsTask

* Minor s removal

* removing wrong modification on protocolhandler

* Renaming restart to consensusTask

* Fixing UT for protocol handler

* Adding comment
@vncoelho vncoelho changed the title Renaming Transaction Verify and Reverify mempool param to transactionsPool Renaming variables related to Consensus Reverification of txs Oct 19, 2019
@vncoelho vncoelho changed the title Renaming variables related to Consensus Reverification of txs Renaming two variables related to Consensus Reverification of txs Oct 19, 2019
@codecov-io
Copy link

Codecov Report

Merging #1162 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1162   +/-   ##
=======================================
  Coverage   65.14%   65.14%           
=======================================
  Files         199      199           
  Lines       13598    13598           
=======================================
  Hits         8858     8858           
  Misses       4740     4740
Impacted Files Coverage Δ
neo/Network/P2P/Payloads/Transaction.cs 82.23% <0%> (ø) ⬆️
neo/Consensus/ConsensusService.cs 15.3% <0%> (ø) ⬆️
neo/Network/P2P/TaskManager.cs 15.64% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4112df2...05014da. Read the comment docs.

@vncoelho vncoelho closed this Oct 23, 2019
@vncoelho
Copy link
Member Author

Closed in favor of the improvements of PR #1183.

However, the renaming of the tasks still looks good.

@vncoelho vncoelho deleted the refactoring-transaction-verify branch October 23, 2019 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigating mempool behavior with similar transactions
3 participants