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.

Sign up for GitHub

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

Jump to bottom

Add geopatch to node transactions #1605

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Add geopatch to node transactions #1605

wants to merge 4 commits into from

Conversation

wassimans
Copy link
Contributor

@wassimans wassimans commented Nov 24, 2024 •
edited
Loading

Description

This pull request introduces the systematic addition of a geopatch to node transactions. This change ensures that the geopatch is included in node transactions and validated during the transaction lifecycle.

The update impacts the election of storage and validation nodes, as it now relies on the validated geopatch from node transactions rather than local calculations. This approach resolves inconsistencies caused by potential updates to the MMDB2 database.

This change introduces:
• The inclusion of the geopatch in node transactions.
• Migration script to add the geopatch to existing nodes transactions.
• Updated tests to validate the geopatch functionality.
• Enhanced core modules for encoding, decoding, and validating geopatches.

Fixes #1576

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Functional Tests

  • Functionally validated the migration script by running it on a network of test nodes.
  • Verified the inclusion of the geopatch in updated transactions and ensured proper validation in subsequent operations.
  • Transaction Content Validation: Validated that node transactions include geopatch information and that it is correctly decoded during transaction processing.
  • Invalid Geopatch Handling: Ensured that transactions with invalid geopatches are rejected.

Unit Tests

  • Enhanced existing test cases for Node.encode_transaction_content/1 and Node.decode_transaction_content/1 to include geopatch validation.
  • Added new tests in GeoPatchTest for geopatch calculation and decoding.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@wassimans wassimans added P2P Involve P2P networking enhancements election labels Nov 24, 2024
@wassimans wassimans requested a review from Neylix November 24, 2024 18:24
@wassimans wassimans force-pushed the feature/add-geopatch-in-node-transaction branch from 98184eb to 923859f Compare November 25, 2024 08:01
bchamagne and others added 3 commits November 25, 2024 17:45
@bchamagne @Neylix
Add the genesis address in the validation_stamp (archethic-foundation…
2813bb7 
…#1588)

* add stamp.genesis_address

* migration: add genesis to io

* code_change

* add genesis to graphql

* add genesis to explorer

* add genesis column to transactions list

* set the genesis before decoding columns

* code_change fix

* remove opts from deserialize

* add missing aliases

* update graphql desc

* Add genesis address only if requested

* Hide genesis address in mobile view

---------

Co-authored-by: Neylix <[email protected]>
@Neylix
Use Task.Supervisor in list_current_summaries
43f8d26
@Neylix
Update node version and dependencies version
886446a
Neylix
Neylix requested changes Nov 27, 2024
View reviewed changes
lib/archethic/bootstrap.ex Outdated
@@ -175,6 +177,7 @@ defmodule Archethic.Bootstrap do
configured_reward_address
) do
Logger.info("Bootstrapping starting")
geo_patch = GeoPatch.from_ip(ip)
Copy link
Member

Choose a reason for hiding this comment

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

You could retrieve the geo patch earlier and use it to determine if the node needs to be updated in the function should_bootstrap? as the node needs to to a transaction if the geopatch is different than the previous one

lib/archethic/p2p/node.ex Outdated
defp extract_mining_public_key(<<>>), do: {nil, <<>>}

defp extract_mining_public_key(rest) do
{mining_public_key, remaining} = Utils.deserialize_public_key(rest)
Copy link
Member

Choose a reason for hiding this comment

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

You could directly return the result of Utils.deserialize_public_key

mix.exs Outdated
@@ -4,7 +4,7 @@ defmodule Archethic.MixProject do
def project do
[
app: :archethic,
version: "1.5.13",
version: "1.5.14",
Copy link
Member

Choose a reason for hiding this comment

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

You should not update the version in this PR, it will be done in a specific PR

test/archethic/mining/distributed_workflow_test.exs Outdated
@@ -139,6 +141,10 @@ defmodule Archethic.Mining.DistributedWorkflowTest do
P2P.authorized_and_available_nodes()
)

stub(MockGeoIP, :get_coordinates, fn
Copy link
Member

Choose a reason for hiding this comment

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

You could add the mock in the setup do you don't need to put it in all the tests

test/archethic/mining/pending_transaction_validation_test.exs
origin_public_key: origin_public_key,
key_certificate: certificate,
mining_public_key: Crypto.generate_random_keypair(:bls) |> elem(0),
geo_patch: "F1B"
Copy link
Member

Choose a reason for hiding this comment

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

You could add a test to ensure the function returns an error of the geopath in the transaction is not the expected one

priv/migration_tasks/prod/1.5.14@add_geopatch_node_transactiosn.ex Outdated
node_pk = node.first_public_key

if node_pk == current_node_pk do
Logger.info("Starting migration for: #{Base.encode16(node_pk)}")
Copy link
Member

Choose a reason for hiding this comment

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

You could use metadata of the Logger module to specify the node public key:

Logger.info("Starting migration", node: Base.encode16(node_pk))

You can find available metadata in config/config.exs:30

priv/migration_tasks/prod/1.5.14@add_geopatch_node_transactiosn.ex Outdated
genesis_address = Crypto.first_node_public_key() |> Crypto.derive_address()

{:ok, %Transaction{data: %TransactionData{code: code}}} =
TransactionChain.get_last_transaction(genesis_address, data: [:code])
Copy link
Member

Choose a reason for hiding this comment

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

P2P.get_node_info returns the last_address of the node in the Node struct. You could directly use this last_address using the function TransactionChain.get_transaction()

priv/migration_tasks/prod/1.5.14@add_geopatch_node_transactiosn.ex Outdated
{:halt, {:error, reason}}
end
else
PubSub.register_to_new_transaction_by_type(:node)
Copy link
Member

Choose a reason for hiding this comment

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

Before registering to pubsub you could check if the last transaction of the node does not already contains a geopatch

priv/migration_tasks/prod/1.5.14@add_geopatch_node_transactiosn.ex Outdated
end)
end

defp process_transaction(transaction) do
Copy link
Member

Choose a reason for hiding this comment

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

I don't get the purpose of this function since in any case do the same instruction and return {:cont, :ok}

priv/migration_tasks/prod/1.5.14@add_geopatch_node_transactiosn.ex Outdated
TransactionChain.get_transaction(address) do
first_pk = TransactionChain.get_first_public_key(previous_pk)

if first_pk == node_pk do
Copy link
Member

Choose a reason for hiding this comment

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

I think if you receive a transaction from an other node than expected you should have a specific behavior. For example if you are waiting for Node1 transaction and you receive Node2 transaction, you will continue in the list and then wait for Node2 transaction, but you just received it

@wassimans wassimans force-pushed the feature/add-geopatch-in-node-transaction branch from 923859f to 5c8b269 Compare November 29, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Neylix Neylix Neylix requested changes

Assignees
No one assigned
Labels
election enhancements P2P Involve P2P networking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Geopatch in node transaction
3 participants
@wassimans @Neylix @bchamagne