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

Unconfirmed blocks #502

Merged
merged 7 commits into from
Aug 8, 2019
Merged

Unconfirmed blocks #502

merged 7 commits into from
Aug 8, 2019

Conversation

karlb
Copy link
Contributor

@karlb karlb commented Aug 6, 2019

This is the main work for #239.

Todos left for other PRs:

  • rename closing_tx_hash
  • check rescheduled block in test
  • check receipts on confirmed block

@karlb karlb requested a review from palango August 6, 2019 15:27
Add test for ActionMonitoringTriggeredEvent rescheduling. One TODO
intentionally left for later.
@karlb karlb force-pushed the unconfirmed-blocks branch from c985754 to fc0c80b Compare August 6, 2019 15:29
@codecov
Copy link

codecov bot commented Aug 6, 2019

Codecov Report

Merging #502 into master will decrease coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #502      +/-   ##
==========================================
- Coverage   91.26%   91.13%   -0.13%     
==========================================
  Files          35       36       +1     
  Lines        2220     2233      +13     
  Branches      282      283       +1     
==========================================
+ Hits         2026     2035       +9     
- Misses        146      148       +2     
- Partials       48       50       +2
Impacted Files Coverage Δ
src/monitoring_service/service.py 90.1% <ø> (-0.11%) ⬇️
src/pathfinding_service/constants.py 100% <ø> (ø)
src/monitoring_service/constants.py 100% <ø> (ø) ⬆️
src/pathfinding_service/service.py 90.71% <100%> (-1.14%) ⬇️
src/raiden_libs/constants.py 100% <100%> (ø)
src/raiden_libs/blockchain.py 96% <100%> (+0.1%) ⬆️
src/pathfinding_service/api.py 95.74% <100%> (+0.05%) ⬆️
src/pathfinding_service/cli.py 95.23% <100%> (ø) ⬆️
src/pathfinding_service/model/channel_view.py 95.91% <100%> (ø) ⬆️
src/monitoring_service/events.py 100% <100%> (ø) ⬆️
... and 6 more

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 6bdba1a...8bed10b. Read the comment docs.

Copy link
Contributor

@palango palango left a comment

Choose a reason for hiding this comment

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

Nice PR, left some small comments.

src/monitoring_service/handlers.py Show resolved Hide resolved
src/monitoring_service/handlers.py Outdated Show resolved Hide resolved
src/monitoring_service/handlers.py Outdated Show resolved Hide resolved
monitor_request.reward_proof_signature,
).transact({"from": context.ms_state.address})
)
), "This MS already monitored this channel. Should be impossible."
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be triggered somehow? Like:

  • MS has a BP, and call monitor with it, thus closing_tx_hash is set
  • We receive a new MonitorRequest
  • Monitoring is triggered again

It's quite constructed because the client would need to be online...

Copy link
Contributor Author

@karlb karlb Aug 8, 2019

Choose a reason for hiding this comment

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

If we receive a new MR after calling monitor, should we call monitor again with the updated BP? That would increase our gas cost. But otherwise a different MS might do that and we will get no reward, although we already have spent gas.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess in general this scenario is not likely.

And what you describe sounds like a loose/loose situation, not sure how we get out of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #504.

@palango palango mentioned this pull request Aug 7, 2019
karlb added 4 commits August 8, 2019 09:55
...and use UDC_SECURITY_MARGIN_FACTOR, instead.
for consistency with `monitoring_service/constants.py` and
`raiden/constants.py`
@karlb karlb merged commit 49892d7 into raiden-network:master Aug 8, 2019
@karlb karlb deleted the unconfirmed-blocks branch August 8, 2019 09:35
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.

2 participants