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

lite block fixes #730

Merged
merged 12 commits into from Feb 24, 2019

Conversation

Projects
None yet
4 participants
@michael-herwig
Copy link
Contributor

commented Feb 6, 2019

This is how far I got until a friend shown up that wanna drink with me :). Please have a look and give some feedback.

This should fix all lite block issues except the alternative chain problem as the missing transactions notification still pushes only into the transaction pool and some transactions may still are missing that would be valid on alternative chains.

Show resolved Hide resolved src/CryptoNoteCore/Core.cpp Outdated
Show resolved Hide resolved src/CryptoNoteProtocol/CryptoNoteProtocolHandler.cpp Outdated
@brandonlehmann

This comment has been minimized.

Copy link
Collaborator

commented Feb 7, 2019

For clarity, this should resolve #727 and #728 correct?

@michael-herwig

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

yes and partially #729, will add some explanation tomorrow

@brandonlehmann

This comment has been minimized.

Copy link
Collaborator

commented Feb 7, 2019

Awesome. Good work!

Michael Herwig
[P2P] notify missing transactions will now check for pending lite blocks
If we acquired missing transactions for a lite block we will be notified by new transactions. We now check if that is a result of a previous lite block request and if so we pass the transactions into the lite block push routine with additional transactions that are not locally available but provided by the publisher.

@michael-herwig michael-herwig changed the title WIP: lite block fixes lite block fixes Feb 7, 2019

@michael-herwig

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

I think this should do it for the three tickets. I didn't test it yet but the implementation should do the following things now:

  • missing transactions of a lite block will only be requested from the publisher
  • if a missing transaction request is sent the initial lite block receiver caches the lite block request as well as the request of the missing transaction
  • if a node gets a new transactions it checks if there is a cached pending lite block request for this peer, if so the transactions are not considered as new pool transaction. Instead, they are pushed into the lite block routine.
  • the lite block routine now can handle an additional context of provided transactions, it will search for provided transactions, blockchain transaction and pool transaction to fill all necessary transaction details.
  • the push lite block routine has some additional sanity checks for checking if all requested missing transactions were provided or a peer tries to push a new lite block while one is already pending

All changes should be backward compatible with older nodes.

@zpalmtree
Copy link
Collaborator

left a comment

Looks good, just a few style tweaks I'd like.

Show resolved Hide resolved src/CryptoNoteProtocol/CryptoNoteProtocolHandler.cpp Outdated
Show resolved Hide resolved src/CryptoNoteProtocol/CryptoNoteProtocolHandler.cpp Outdated
Show resolved Hide resolved src/CryptoNoteProtocol/CryptoNoteProtocolHandler.cpp Outdated
Show resolved Hide resolved src/CryptoNoteProtocol/CryptoNoteProtocolHandler.cpp Outdated
Show resolved Hide resolved src/CryptoNoteProtocol/CryptoNoteProtocolHandler.cpp Outdated
@zpalmtree

This comment has been minimized.

Copy link
Collaborator

commented Feb 14, 2019

Looks good. Thanks for the PR! Do we want to spin up a testnet @brandonlehmann @RocksteadyTC ?

@brandonlehmann

This comment has been minimized.

Copy link
Collaborator

commented Feb 14, 2019

Probably a good idea.

MaddestHatter added a commit to MaddestHatter/excelsior that referenced this pull request Feb 15, 2019

@RocksteadyTC

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

I'm free for testnet today if we want to give this a shot.

@brandonlehmann brandonlehmann merged commit a317b74 into turtlecoin:development Feb 24, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.