Conversation
vncoelho
left a comment
There was a problem hiding this comment.
Let me just check with careful asap.
This will open possibilities for other improvements. Nice job.
lock9
left a comment
There was a problem hiding this comment.
Very good. No tests, but there was no change in logic. I will approve this PR, but please try to make more tests, this way I can run your code here 👍
|
I think that it would be good for take decisions according to the view change. For example, if we have 5 view changes because policy, maybe i need to clean my memory pool? |
|
@shargon That is a good idea! When you had that method to clean added to the ProtocolHandler, I already had in mind that this "clean" should be automated, just like most apps are (if you get a memory warning, you need to clean resources). So, yes, I think that this "inconsistency" of the network may lead nodes to clean their data. What do you guys think? I think this is a good idea, not sure about side effects, especially for Nash Edit: But if we proceed, please create another issue 😄 |
Codecov Report
@@ Coverage Diff @@
## master #926 +/- ##
==========================================
- Coverage 45.35% 45.35% -0.01%
==========================================
Files 178 178
Lines 12628 12637 +9
==========================================
+ Hits 5728 5731 +3
- Misses 6900 6906 +6
Continue to review full report at Codecov.
|
neo/Consensus/ConsensusService.cs
Outdated
| // if my last change view payload, `message`, has NewViewNumber lower than current view to change | ||
| if (message is null || message.NewViewNumber < viewNumber) | ||
| localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeChangeView() }); | ||
| localNode.Tell(new LocalNode.SendDirectly { Inventory = context.MakeChangeView(ChangeViewReason.WrongExpectedView) }); |
There was a problem hiding this comment.
I think it sends ChangeView because everyone wants to change view. There is nothing wrong.
There was a problem hiding this comment.
Hi @erikzhang Yoda master,
I think that if the reason is something like invalid tx there will be no reason for recovery, for example.
There was a problem hiding this comment.
@erikzhang, my comment above was wrong...
I just remembered now that we already created a special payload RecoveryRequest.
In this sense, I am not sure about a real use of this Reason.
Should we revert, @shargon? Or do you have an idea for using it?
@igormcoelho
* Change view reason * Move order for compatibility * Moved to other file * Change order * Change agreement
Follow neo-project/neo#926 for Neo 3 (though the change is compatible with Neo 2).
Follow neo-project/neo#926 for Neo 3 (though the change is compatible with Neo 2).
Follow neo-project/neo#926 for Neo 3 (though the change is compatible with Neo 2).
Follow neo-project/neo#926 for Neo 3 (though the change is compatible with Neo 2).
TODO: We can review if the most common reason was
TxNotFoundand reduce the next block proposal from 500 tx to 250.