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

feat!: add custom noise prologue to comms nodes #6502

Conversation

hansieodendaal
Copy link
Contributor

@hansieodendaal hansieodendaal commented Aug 22, 2024

Description

Added a custom noise prologue to the noise protocol to be hashed into the handshake hash value. This enables an application to specify a unique identifier like the genesis block hash as the noise prologue, effectively stopping any communication handshake from succeeding with nodes not on the same genesis block.

Thanks to @sdbondi for brainstorming and suggesting the way this requirement could be implemented.

Motivation and Context

Base nodes and wallets maintained numerous connections from which they could not sync or obtain useful information. This caused base nodes to try and follow other base nodes that were on a higher proof of work but on a different genesis block, and caused wallets to query base nodes that supplied information from a different blockchain.

How Has This Been Tested?

Passed all unit tests.
Passed system-level testing.

What process can a PR reviewer use to test or verify this change?

Code review.
System-level testing.

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

BREAKING CHANGE: Communication between base nodes and wallets will be exclusively linked to the genesis block hash once this PR is implemented. They will not be able to communicate to any previous release.

Added a custom noise prologue to the noise protocol to be hashed in to the handshake hash value.
This enables an application to specify a unique identifier like the genesis block hash as the
noise prologue, effectively stopping any communication handshake to succeed with nodes that is
not on the same genesis block.
@hansieodendaal hansieodendaal requested a review from a team as a code owner August 22, 2024 16:05
Copy link

github-actions bot commented Aug 22, 2024

Test Results (CI)

    3 files    126 suites   38m 55s ⏱️
1 307 tests 1 307 ✅ 0 💤 0 ❌
3 919 runs  3 919 ✅ 0 💤 0 ❌

Results for commit 3d77336.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Aug 22, 2024
Copy link

github-actions bot commented Aug 22, 2024

Test Results (Integration tests)

 2 files  + 2  11 suites  +11   25m 56s ⏱️ + 25m 56s
37 tests +37  35 ✅ +35  0 💤 ±0  2 ❌ +2 
39 runs  +39  37 ✅ +37  0 💤 ±0  2 ❌ +2 

For more details on these failures, see this check.

Results for commit 3d77336. ± Comparison against base commit e734269.

♻️ This comment has been updated with latest results.

@AaronFeickert
Copy link
Collaborator

Would this handle the case of an intentional chain fork that uses the same genesis block?

@hansieodendaal
Copy link
Contributor Author

Would this handle the case of an intentional chain fork that uses the same genesis block?

Yes. P2pInitializer::noise_prologue is an open byte vector, so one could easily add some consensus constant differentiator to it that will activate at a certain date and time. There could be a glitch in communication for a very short period before and after that time, but it will be rectified once the majority of nodes move past that.

@hansieodendaal hansieodendaal changed the title feat: add custom noise prologue to comms nodes feat!: add custom noise prologue to comms nodes Aug 23, 2024
sdbondi
sdbondi previously approved these changes Aug 23, 2024
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

utACK

applications/minotari_node/src/bootstrap.rs Show resolved Hide resolved
brianp
brianp previously approved these changes Aug 23, 2024
base_layer/contacts/tests/contacts_service.rs Outdated Show resolved Hide resolved
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

utACK

comms/core/src/connectivity/manager.rs Outdated Show resolved Hide resolved
common/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

I am very much against the idea of using the gen block in the handshake. We have the wire mode, we can use that to stop wallets and base nodes to not talk to each other on different networks. This seems like an over engineered solution to a problem we already have a solution to.

BUT
My biggest issue is this. When we launch mainnet, we need to be able to compile the wallets ahead of time. Especially mobile wallets that need a minimum of 3 days lead time to go through app approval process. If we merge this pr, we will launch without any mobile wallets for 3 days.

@AaronFeickert
Copy link
Collaborator

One option could be to do something akin to Zcash's branch IDs, which are an opt-in way for intentional chain forks to offer replay protection, and which was also proposed for handshake binding. Forks simply choose an arbitrary but globally-unique brand ID, which is bound to transactions for replay protection, and which can be bound to the handshake for peer interaction.

@hansieodendaal
Copy link
Contributor Author

I am busy changing the PR to be activated by consensus, espescially for mainnet. It will start with a default block hash, and then at any later date, it will be replaced by the actual block hash. Consensus (as use_handshake_prologue) will decide if and when the block hash (as the custom handshake noise prologue) will be used. Together with this, I am adding a prologue suffix (as handshake_prologue_suffix) to the consensus constants, which when added to the block hash, could be used to differentiate network communication after a soft fork. This approach seems to be somewhat similar to Zcash's branch IDs.

@hansieodendaal hansieodendaal mentioned this pull request Sep 2, 2024
4 tasks
SWvheerden pushed a commit that referenced this pull request Sep 2, 2024
Description
---
Added network wire byte. This is now independent of the network as byte.

Note: This method in favour of #6502.

Motivation and Context
---
Base nodes and wallets maintained numerous connections from which they
could not sync or obtain useful information. This caused base nodes to
try and follow other base nodes that were on a higher proof of work but
on a different genesis block, and caused wallets to query base nodes
that supplied information from a different blockchain. This byte can now
be changed every time the genesis block changes.

How Has This Been Tested?
---
Passed all unit tests.

What process can a PR reviewer use to test or verify this change?
---

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [X] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants