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

refactor(side-dag): general improvements #1080

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

glevco
Copy link
Contributor

@glevco glevco commented Jul 8, 2024

Motivation

Implement some of the requested changes in previous PRs.

Acceptance Criteria

  • Remove poa.in_turn_signer_index() and add poa.get_signer_index_distance().
  • Update poa.calculate_weight() to decrease the block weight inversely proportional to the index distance.
  • Changes in PoaBlockProducer:
    • Merge both looping calls into one.
    • Add delayed block production cancellation.
    • Update logs.
    • Change block delay function to include a constant offset for out of turn blocks, and increase the interval between each signer.
  • Update tests accordingly.

Checklist

  • If you are requesting a merge into master, confirm this code is production-ready and can be included in future releases as soon as it gets merged

@glevco glevco added the refactor label Jul 8, 2024
@glevco glevco self-assigned this Jul 8, 2024
@glevco glevco force-pushed the refactor/side-dag/improvements branch from cc30326 to c1d33fa Compare July 8, 2024 21:31
@glevco glevco force-pushed the refactor/side-dag/multiprocessing branch 3 times, most recently from 02792c0 to e965a13 Compare July 11, 2024 15:25
@glevco glevco changed the title Refactor/side dag/improvements refactor(side-dag): general improvements Jul 11, 2024
@glevco glevco force-pushed the refactor/side-dag/multiprocessing branch from 5da8d8c to ef14b24 Compare July 18, 2024 18:13
@glevco glevco force-pushed the refactor/side-dag/improvements branch 2 times, most recently from 10949f1 to 7b24072 Compare July 18, 2024 19:47
@glevco glevco force-pushed the refactor/side-dag/improvements branch from 7b24072 to ad34b3f Compare July 18, 2024 20:39
@glevco glevco marked this pull request as ready for review July 18, 2024 20:53
@glevco glevco requested review from jansegre and msbrogli as code owners July 18, 2024 20:53
@glevco glevco force-pushed the refactor/side-dag/multiprocessing branch 5 times, most recently from ee73bbf to e140836 Compare July 19, 2024 21:43
@glevco glevco force-pushed the refactor/side-dag/improvements branch from ad34b3f to 713b3e2 Compare July 19, 2024 21:56
Copy link

codecov bot commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 83.67347% with 8 lines in your changes missing coverage. Please review.

Project coverage is 85.00%. Comparing base (7829afb) to head (c605607).

Files Patch % Lines
hathor/consensus/poa/poa_block_producer.py 80.48% 5 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1080      +/-   ##
==========================================
- Coverage   85.08%   85.00%   -0.08%     
==========================================
  Files         314      314              
  Lines       23937    23957      +20     
  Branches     3616     3621       +5     
==========================================
- Hits        20367    20365       -2     
- Misses       2867     2879      +12     
- Partials      703      713      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

msbrogli
msbrogli previously approved these changes Jul 22, 2024
jansegre
jansegre previously approved these changes Jul 22, 2024
@glevco glevco force-pushed the refactor/side-dag/multiprocessing branch from e140836 to 81fd0ce Compare July 22, 2024 19:31
@glevco glevco force-pushed the refactor/side-dag/improvements branch from 713b3e2 to 576b4ac Compare July 22, 2024 19:32
@glevco glevco force-pushed the refactor/side-dag/multiprocessing branch from 81fd0ce to 5e46ebd Compare July 22, 2024 21:05
@glevco glevco force-pushed the refactor/side-dag/improvements branch from 576b4ac to c4e2dea Compare July 22, 2024 21:09
@glevco
Copy link
Contributor Author

glevco commented Jul 22, 2024

I had to make some changes because I noticed the behavior was not working as expected, in relation to cancellation of block production. Before these changes,

  • Since delayed calls were not cancelled when the delayed block was in turn, that meant we could have more than one active delayed call, and only one of them would have a pointer (in self._delayed_call).
  • In fact, the block used to check the turn was incorrect, as it was using the delayed call argument which is the previous_block, not the new one being generated.
  • We were not cancelling block production when a new best block arrived from another peer.

All these issues have been addressed in a separate commit, 37ce560.

Base automatically changed from refactor/side-dag/multiprocessing to master July 23, 2024 15:11
@glevco glevco dismissed stale reviews from jansegre and msbrogli July 23, 2024 15:11

The base branch was changed.

@glevco glevco force-pushed the refactor/side-dag/improvements branch from 452b00e to 56f9987 Compare July 23, 2024 15:13
@jansegre jansegre mentioned this pull request Jul 25, 2024
2 tasks
@glevco glevco force-pushed the refactor/side-dag/improvements branch 2 times, most recently from 0abdc8d to e652a8b Compare July 26, 2024 18:50
jansegre
jansegre previously approved these changes Jul 26, 2024
@glevco glevco force-pushed the refactor/side-dag/improvements branch from e652a8b to 101c8dd Compare July 26, 2024 20:43
msbrogli
msbrogli previously approved these changes Jul 26, 2024

def _schedule_block(self) -> None:
"""Schedule propagation of a new block."""
if not self.manager.can_start_mining():
# We're syncing, so we'll try again later
self._log.info('cannot produce new block, node not synced')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that the full node would not recover from getting out of sync after it was already synced. From recovery, I mean that this full node would stop producing blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The full node recovers as long as it receives a block from another peer. I added an assertion in an existing test to cover this, in 0d4fea6

self._delayed_call: IDelayedCall | None = None
self._start_producing_lc: LoopingCall = LoopingCall(self._safe_start_producing)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand why we need this LoopingCall. Anyway, I'm ok to get this PR merged and get this changed later because it has no impact on mainnet and testnet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using either a LoopingCall or a recursive callLater, separate from the main callLater logic (triggered reactively via pubsub) were the only good ways I found to implement this while covering the following requirements:

  • A single full node should be able to produce blocks by itself.
  • If block production throws during the "start producing" phase, production should be tried again. However, if it fails after a call via pubsub, it should not be retried.
  • We have to consider that the call to can_start_mining() may throw an exception.

I created an issue to review this later: #1097

@glevco glevco force-pushed the refactor/side-dag/improvements branch from c0203e5 to c605607 Compare July 26, 2024 21:47
@jansegre jansegre merged commit 716a012 into master Jul 26, 2024
11 of 12 checks passed
@jansegre jansegre deleted the refactor/side-dag/improvements branch July 26, 2024 23:12
@jansegre jansegre mentioned this pull request Aug 1, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants