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

fix: fix panic when no public addresses #5367

Merged

Conversation

stringhandler
Copy link
Collaborator

@stringhandler stringhandler commented May 5, 2023

Description

If there are no public addresses specified, there was a panic. This is solved by not starting a liveness service if there is no public address.

NOTE: I have changed the reference on tari-crypto to be the lowest version that it works with (i.e. 0.16, instead of 0.16.*). This is the best practice for libraries, according to Rust, and also helps with mismatches on the tari-dan repo.

Motivation and Context

When using TCP, a node might not have a public address, this may cause a panic with the current code. AFAIK a comms node (base node, wallet, dan indexer) can still work without a public address, but it can only initiate outbound connections.

How Has This Been Tested?

existing tests

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

TODO: I'm busy checking this on the DAN indexer, which has no public address during testing

Breaking Changes

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

sdbondi
sdbondi previously approved these changes May 5, 2023
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

@ghpbot-tari-project ghpbot-tari-project added the P-acks_required Process - Requires more ACKs or utACKs label May 5, 2023
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.

Looks good, I would just like that one unwrap rather return an error

base_layer/p2p/src/initialization.rs Show resolved Hide resolved
@ghpbot-tari-project ghpbot-tari-project added the P-reviews_required Process - Requires a review from a lead maintainer to be merged label May 8, 2023
@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label May 10, 2023
@stringhandler stringhandler merged commit 49be2a2 into tari-project:development May 10, 2023
10 of 11 checks passed
@stringhandler stringhandler deleted the st-public-address-panic branch May 10, 2023 11:49
SWvheerden added a commit that referenced this pull request May 29, 2023
##
[0.50.0-pre.2](v0.50.0-pre.1...v0.50.0-pre.2)
(2023-05-29)

### ⚠ BREAKING CHANGES

* add optional range proof types (5372)

### Features

* add metadata signature check
([5411](#5411))
([9c2bf41](9c2bf41))
* add optional range proof types
([5372](#5372))
([f24784f](f24784f))
* added burn feature to the console wallet
([5322](#5322))
([45685b9](45685b9))
* improved base node monitoring
([5390](#5390))
([c704890](c704890))


### Bug Fixes

* **comms:** only set final forward address if configured to port 0
([5406](#5406))
([ff7fb6d](ff7fb6d))
* deeplink to rfc spec
([5342](#5342))
([806d3b8](806d3b8))
* don't use in memory datastores for chat client dht in integration
tests ([#5399](#5399))
([cbdca6f](cbdca6f))
* fix panic when no public addresses
([5367](#5367))
([49be2a2](49be2a2))
* loop on mismatched passphrase entry
([5396](#5396))
([ed120b2](ed120b2))
* use domain separation for wallet message signing
([5400](#5400))
([7d71f8b](7d71f8b))
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants