Skip to content

fix: restore multinode mode functionality#1031

Merged
Nana-EC merged 2 commits intohiero-ledger:mainfrom
isavov:1017-multinode-fix
May 9, 2025
Merged

fix: restore multinode mode functionality#1031
Nana-EC merged 2 commits intohiero-ledger:mainfrom
isavov:1017-multinode-fix

Conversation

@isavov
Copy link
Copy Markdown
Member

@isavov isavov commented Apr 17, 2025

Description:

Changes introduced in the consensus node version 0.59.0 have resulted in multinode mode not working. This PR restores the functionality by:

  • Converting the old pfx formatted keys and certificates for the additional nodes to pem.
  • Updating config.multinode.txt with settings compatible with the newer versions
  • Updating ENV variables for the additional nodes.

Related issue(s):

Fixes #1017, Fixes #992

Notes for reviewer:

I did my best to test this PR, but was not able to test it fully. Please test yourself before merging.

Limited by the technology of my time
Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@isavov isavov requested a review from a team as a code owner April 17, 2025 11:53
@isavov isavov requested a review from georgi-l95 April 17, 2025 11:53
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1031   +/-   ##
=======================================
  Coverage        ?   60.21%           
=======================================
  Files           ?       29           
  Lines           ?     1204           
  Branches        ?      153           
=======================================
  Hits            ?      725           
  Misses          ?      456           
  Partials        ?       23           

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
@isavov isavov force-pushed the 1017-multinode-fix branch from 01adebb to aeec8e8 Compare April 17, 2025 11:56
Nana-EC
Nana-EC previously approved these changes Apr 28, 2025
Copy link
Copy Markdown
Contributor

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

Great work. Minor non blocking comments.
I had a PR with similar changes I wanted to push but your changes are focused so let's go with this.
if this works let's get it in and get a new version out and I can circle back with tickets for other things I had in mind.
@georgi-l95 kindly take a look.

@Nana-EC
Copy link
Copy Markdown
Contributor

Nana-EC commented May 6, 2025

Bump @isavov and @georgi-l95

@Nana-EC Nana-EC added the Bug An error that causes the feature to behave differently than what was expected based on design. label May 6, 2025
@isavov
Copy link
Copy Markdown
Member Author

isavov commented May 8, 2025

@Nana-EC @georgi-l95 Rereview please 🙏
Pushed some updates, needed to adjust the multinode config - it appears nodes should be defined by ip addresses and not FQDNs.
The other issue was related to mirror node monitor, which needed updates for post 0.59.0 consensus nodes.
In my tests I got some issues with timeouts while starting up, which were resolved by setting the retry attempt to a higher value from the default 100, likes so
FIRING_UP_RETRY_ATTEMPTS=1000 npm run restart -- -d --verbose=trace --multinode

@Nana-EC
Copy link
Copy Markdown
Contributor

Nana-EC commented May 8, 2025

@Nana-EC @georgi-l95 Rereview please 🙏 Pushed some updates, needed to adjust the multinode config - it appears nodes should be defined by ip addresses and not FQDNs. The other issue was related to mirror node monitor, which needed updates for post 0.59.0 consensus nodes. In my tests I got some issues with timeouts while starting up, which were resolved by setting the retry attempt to a higher value from the default 100, likes so FIRING_UP_RETRY_ATTEMPTS=1000 npm run restart -- -d --verbose=trace --multinode

Hey @isavov , nice work.
Just to be clear can you confirm you've been able to publish transactions using the mirror node monitor to all 4 nodes without issues and the transactions went through consensus?
If so then everything is solid 🙌🏾

Nana-EC
Nana-EC previously approved these changes May 8, 2025
Copy link
Copy Markdown
Contributor

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LG
If we can confirm operation then I think we're good to get this in and unblock users for this mode, then iterate as needed

Signed-off-by: Iliya Savov <isavov@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

LG.
Let's get this in and iterate

Copy link
Copy Markdown
Member

@georgi-l95 georgi-l95 left a comment

Choose a reason for hiding this comment

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

Works, played around by stoping and starting different nodes to see if they catch up or if the mirror node ingests the record files. Everything is working as expected. Well done and thank you @isavov

@Nana-EC Nana-EC merged commit 804866d into hiero-ledger:main May 9, 2025
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug An error that causes the feature to behave differently than what was expected based on design.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multinode option is not working Local node starts only a single network-node with --multinode option

3 participants