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

Replicate only missing transactions #443 #453

Merged

Conversation

apoorv-2204
Copy link
Contributor

Description

Fixes # (#443)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

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

@apoorv-2204 apoorv-2204 requested a review from Neylix July 17, 2022 15:04
Copy link
Member

@Neylix Neylix left a comment

Choose a reason for hiding this comment

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

The process to get the last local transaction address should also be done in the replication process. Maybe in TransactionContext.stream_transaction_chain

lib/archethic.ex Outdated
"""
@spec get_last_transaction_address_locally(address :: binary()) ::
{:ok, binary()} | {:error, :not_found} | {:error, :network_issue}
def get_last_transaction_address_locally(address) when is_binary(address) do
Copy link
Member

Choose a reason for hiding this comment

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

This function may be in TransactionChain module, same for get_genesis_transaction_address, fetch_transaction_chain_locally and available_loccaly? as we request an information of a transaction chain

lib/archethic.ex Outdated
def available_locally?(address) when is_binary(address) do
case get_last_transaction_address_locally(address) do
{:ok, last_address} -> {true, last_address}
{:error, :not_found} -> {false, nil}
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 also catch {:error, :network_issue}
Also in case of wrong result, you can juste return false, nil is not used after

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right

lib/archethic.ex Outdated

chain_page =
paging_address
|> TransactionChain.stream_remotely(nodes)
Copy link
Member

Choose a reason for hiding this comment

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

This function aims to get the local transaction chain, but you get it remotely so it's not the local chain.
You should use TransactionChain.get

Copy link
Contributor Author

@apoorv-2204 apoorv-2204 Jul 18, 2022

Choose a reason for hiding this comment

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

I am querying the same node via P2P.message abstraction.
It twill be more secure rather than directly querying the DB.

@doc """
Retrieve the genesis address of a chain
"""
@spec resolve_last_address(binary()) :: {:ok, binary()} | {:error, :network_issue}
Copy link
Member

Choose a reason for hiding this comment

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

Spec definition so not martch the function name.
Same for fetch_genesis_address_remotely

@apoorv-2204 apoorv-2204 marked this pull request as ready for review July 18, 2022 12:12
@apoorv-2204 apoorv-2204 self-assigned this Jul 18, 2022
@apoorv-2204 apoorv-2204 added bug Something isn't working P2P Involve P2P networking transaction Involve transactions DB Involve database testing Improve testing quality Improve code quality labels Jul 18, 2022
@apoorv-2204 apoorv-2204 requested a review from Neylix July 18, 2022 17:20
- included new method fetch chain efficiently
- add checks before fetching txn chain
- resolve tests
@apoorv-2204 apoorv-2204 removed the quality Improve code quality label Jul 18, 2022
@Neylix Neylix merged commit d244b1d into archethic-foundation:develop Jul 20, 2022
@apoorv-2204 apoorv-2204 deleted the replicate_txn_chain_443 branch July 26, 2022 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working DB Involve database P2P Involve P2P networking testing Improve testing transaction Involve transactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants