-
Notifications
You must be signed in to change notification settings - Fork 2
Feat/new peer discovery #61
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
Conversation
const val PREF_KEY_SELECTED_PEERS_CACHED_AT = "${PREF_KEY_SELECTED_PEERS}_cached_at" | ||
} | ||
|
||
override suspend fun fetchSelectedPeers(): Set<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function will fetch from here https://api.blockchair.com/litecoin/nodes, then if cache didn't expired & not empty then return cachedPeers, if not, will fetch from API then save into local with some filter criteria
currently only filter port 9333
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok @andhikayuana . Please make sure that url can be easily changed to via the remote config a call in the new APIManager. We are very close to making that available.
} | ||
|
||
//wrap logic enable/disable connect with new flow | ||
public void wrapConnectV2() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrap connect function here
.build() | ||
|
||
} | ||
// private val remoteConfigSource: RemoteConfigSource = mockk() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently the test commented
} | ||
|
||
} | ||
// private val remoteConfigSource: RemoteConfigSource = mockk() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently the test commented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you! @andhikayuana
Still reviewing @andhikayuana |
val cachedPeers = sharedPreferences.getStringSet(PREF_KEY_SELECTED_PEERS, null) | ||
|
||
// Check if cache exists and is less than 6 hours old | ||
if (!cachedPeers.isNullOrEmpty() && (currentTime - lastUpdateTime) < 6 * 60 * 60 * 1000) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cache 6hrs
val nodesObject = dataObject?.get("nodes")?.jsonObject | ||
|
||
//filter criteria | ||
val requiredServices = 0x01 or 0x04 // NODE_NETWORK | NODE_BLOOM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add filter criteria
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sync is much better...found an unrelated critical bug: https://github.com/gruntsoftware/internal/issues/119
val dataObject = jsonElement.jsonObject["data"]?.jsonObject | ||
val nodesObject = dataObject?.get("nodes")?.jsonObject | ||
|
||
//TODO: need filter criteria here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @andhikayuana
Maybe you could try:
///Given the node delivers full blocks and has Bloom filters on
fun peerNodeIsUsable(node: Map<String, Any>): Boolean {
val flags = (node["flags"] as? Number)?.toInt() ?: return false
return (flags and 0b101) == 0b101
}
} | ||
|
||
companion object { | ||
const val LITECOIN_NODES_URL = "https://api.blockchair.com/litecoin/nodes" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great for the interim @andhikayuana . My concern is that as we get more users, they will all hit the rate limits. So we are working hard with the backend team to implement an endpoint for Brainwallet mobile clients...issue is here
Then, you can just store the nodes into encrypted preferences
//TODO | ||
interface RemoteApiSource { | ||
|
||
@GET("api/v1/rates") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very close @andhikayuana .
I will had 'v2' in the url for the backend
.addHeader("Content-Type", "application/json") | ||
.addHeader("X-Litecoin-Testnet", "false") | ||
.addHeader("Accept-Language", "en") | ||
// .addHeader("User-agent",) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually need that User-agent
in the query so we can differentiate from iOS, Android, iPhone and iPad @andhikayuana
const val PREF_KEY_SELECTED_PEERS_CACHED_AT = "${PREF_KEY_SELECTED_PEERS}_cached_at" | ||
} | ||
|
||
override suspend fun fetchSelectedPeers(): Set<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok @andhikayuana . Please make sure that url can be easily changed to via the remote config a call in the new APIManager. We are very close to making that available.
Overview
Improve peer discovery, related with gruntsoftware/core#5