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

Permit listening to version messages #36

Conversation

dmos62
Copy link

@dmos62 dmos62 commented Feb 4, 2021

This is the porting of PR #35 to BitcoinJ 15.8. I'll copy-paste the general description:

Since Bisq requires nodes with specific configuration (as reflected in
their version messages), we also want to handle misconfigured nodes,
especially in cases where the user expects that node to be used. To get
access to all received version messages, a new listener is introduced.
Existing listeners are not sufficient, because in some cases BitcoinJ
will kill the Peer (connection to a node) before any of them are
triggered. This listener will always be triggered when a VersionMessage
is received.

Practically no code changes were required to apply this to 15.8. I copy-pasted the changes one by one anyway to make sure, hence different set of commits.

This keeps an erronious protocol exception from being thrown if an
alert message is received before a version message is received.

Fixes bisq-network/bisq#4080
// No further communication is possible until version handshake is complete.
if (!(m instanceof VersionMessage || m instanceof VersionAck
// Most further communication isn't possible until version handshake is complete.
if (!(m instanceof VersionMessage || m instanceof VersionAck || m instanceof AlertMessage
Copy link
Author

@dmos62 dmos62 Feb 4, 2021

Choose a reason for hiding this comment

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

See commit message: 42b32f6 This is a bugfix. Since we've updated to BitcoinJ 15.8, I'm not aware of problems that this bug causes at this time. So this is not strictly related to the PR; however, I think we might as well fix it. Also, I fixed the above comment, since it's in conflict with the next line.

@ripcurlx
Copy link

Could @chimp1984 or @stejbac review this PR who more familiar with the bitcoinj code? Thanks!

@dmos62 dmos62 closed this Feb 28, 2022
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