-
Notifications
You must be signed in to change notification settings - Fork 717
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
[Validation] Synchronous update for CDeterministicMNManager::tipIndex #2631
[Validation] Synchronous update for CDeterministicMNManager::tipIndex #2631
Conversation
13d59a2
to
74e021e
Compare
Concept ACK, nice catch the Have been investigating the not found DMN payment failure reason, and is because the cached index inside the manager is from the previous block (not the tip) when the test calls to Better to move the dmn manager tip update call to the bottom of I have run this patch furszy@1ba46ae over 200 times and it did not fail again for a not found DMN payment. Aside from that, the duplicate operator key issue still happens. Which seems to be unrelated to this first problem as it's much more inconsistent (at least locally), have seen it only once in a consecutive 200 times test run. |
d1c6d34
to
d749c3b
Compare
Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
As Init() is called from the background thread it should never rely on CDeterministicMNManager::tipIndex, which instead is updated synchronously
d749c3b
to
bcdb4f0
Compare
Yeah. This was caused by a bug in the bls-signatures library (fixed here: PIVX-Project/bls-signatures#11) This PR is ready for review now. |
Can rebase it on top of #2650 if you want now :). |
No need to. The move operator is fine, and this particular issue wasn't really even related to the changes here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code ACK with some comments.
An extra point would be to rename the DeterministicMNManager::UpdatedBlockTip
function so we don't get confused by the validation interface signal function that has the same name and a different workflow (sync call vs async call). Getting older and tend to forget things like this easily..
01a7048
to
55588dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome to see this PR finally passing 😄, ACK 55588dc
bc84ff0 Doc: add bls-sigs repo to developer-notes subtrees section (furszy) 9daeb29 Squashed 'src/chiabls/' changes from bc64f128f0..eaf7d2d1a4 (furszy) Pull request description: Updating the bls-signatures subtree, reason is PIVX-Project/bls-signatures#11 (which gladly was detected before v6.0). Coming from #2631 investigation. Pulling up to PIVX-Project/bls-signatures@eaf7d2d. ACKs for top commit: random-zebra: utACK bc84ff0 Tree-SHA512: 91744a317eb8834bdac792a2cfc0fca306bb63798ec037743057c87238087c68b2bceca5811d6fdb9865a8a3ba5f5dd7268bebd398bc2d80223cd3e1baa625bd
As this has direct consensus implications, it cannot be updated in the background thread (in fact, the tests must pass without having to resort to
SyncWithValidationInterfaceQueue
).The manager's tip must be updated on the caller's thread and, on the other hand, what is being called by
UpdatedBlockTip
(on the background), such asCActiveDeterministicMasternodeManager::Init
orCActiveDeterministicMasternodeManager::Reset
, shouldn't rely on the manager's tip, but usepindexNew
instead.