-
Notifications
You must be signed in to change notification settings - Fork 8
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
Refactor #2
Refactor #2
Conversation
moneriote.py
Outdated
@@ -1,303 +1,502 @@ | |||
#!/usr/bin/python3.5 |
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.
My python 3.6.5 can't run with this. But I don't know the ideal way to handle this.
moneriote.py
Outdated
# If we have room in Cloudflare for records, we need to discover nodes | ||
cf_add_count = self.cf_max_records - len(nodes_cf) | ||
if cf_add_count <= 0: | ||
log_msg("No need to add more Cloudflare records") |
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.
If I read this right, it seems your strategy is keeping those max_records until some of them go offline. Then script finds new node to add. However, I prefer the current way that keep all valid node IP in current_node (and file it). Then we random pick max_records numbers of IP and update to CF every period of time. This could decentralize the traffic and reduce the loading of volunteer's node.
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.
Also, I believe keeping all IP is easier to make geo-ip subdomain (my next task).
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.
I agree on the refreshing of existing records. This however, would not require a client side file.
On the topic of geo-ip subdomains; It's a ipv4->geo key/val lookup. Has nothing to do with caching IPs locally. There are Python libs for the Maxmind dump.
The only valid technical reason to use a cache file is to not scan peers multiple times in a short time span. This requirement would also dictate managing when peers have entered the cache file; at which point my interest go towards a Redis solution.
I also have more plans with this repository, would you say I should hard-fork this and continue on my own?
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.
I will do that and leave this PR as is.
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.
I would merge this nice PR if the refreshing of existing record part could be done. But if you have more plans and think hard fork is better for your work then I'll totally OK with that.
The original reason I kept all IP in file is when switching to a new synced node, the peer list would need some time to complete. So keep IP list is more efficient. This often happens when I test this script in early time. But now I agree it might not necessary. And my mentioned geo-ip is the point of keeping all IP, not file, so you're right.
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 I will see what I can do.
moneriote.py
Outdated
nodes = RpcNodeList() | ||
|
||
log_msg('Fetching existing record(s) (%s.%s)' % (self.cf_subdomain_name, self.cf_domain_name)) | ||
for record in self._cf_request('%s/dns_records/' % self.cf_zone_id): |
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.
I got an exception here.
[2018-09-05 20:03] Error contacting Cloudflare (xxxxxxxxxxxxxxxxxxxxx705e181xxxe/dns_records/): HTTPSConnectionPoolhost='api.cloudflare.com', port=443): Read timed out. (read timeout=5)
Traceback (most recent call last):
File "C:\Users\walass\Desktop\opennode\testing\moneriote.py", line 500, in <module>
mon.loop()
File "C:\Users\walass\Desktop\opennode\testing\moneriote.py", line 423, in loop
for record in self._cf_request('%s/dns_records/' % self.cf_zone_id):
TypeError: 'NoneType' object is not iterable
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.
Latest commit breaks things. If you were not using my latest commit, then it's a bug and I will fix it later. Thanks for trying.
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.
It is ba3c5b9. I'll try again after you sort it out :)
What's the status of this PR? i would really like to include this repo to the monro-ecosystem |
@erciccione It is 95% done on my end, I should be able to finish this PR on saturday. |
Perfect, thanks for the update @skftn |
I should be done now. Hope @Lafudoci likes it! |
moneriote.py
Outdated
return val | ||
|
||
cfg_monero = {'md_%s' % k: try_cast(v) for k, v in config._sections.get('MoneroDaemon', {}).items()} | ||
cfg_cloudflare = {'cf_%s' % k: try_cast(v) for k, v in config._sections.get('cloudflareAPI', {}).items()} |
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.
There is an issue here when using --from-config arg. The config file seems changed for more DNS provider. The target of 'cloudflareAPI'
should be now 'DNS'
. But after changing that, I still got this error: TypeError: __init__() got an unexpected keyword argument 'cf_provider'
.
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.
I think you are on an old version. https://github.com/skftn/moneriote-python/blob/refactor/moneriote/utils.py#L81 parses the ini.
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.
(venv) C:\Users\walass\Desktop\moneriote-python-refactor>moneriote.py --from-con
fig config.ini
• ▌ ▄ ·. ▐ ▄ ▄▄▄ .▄▄▄ ▪ ▄▄▄▄▄▄▄▄ .
·██ ▐███▪▪ •█▌▐█▀▄.▀·▀▄ █·██ ▪ •██ ▀▄.▀·
▐█ ▌▐▌▐█· ▄█▀▄ ▐█▐▐▌▐▀▀▪▄▐▀▀▄ ▐█· ▄█▀▄ ▐█.▪▐▀▀▪▄
██ ██▌▐█▌▐█▌.▐▌██▐█▌▐█▄▄▌▐█•█▌▐█▌▐█▌.▐▌ ▐█▌·▐█▄▄
▌
▀▀ █▪▀▀▀ ▀█▄▀▪▀▀ █▪ ▀▀▀ .▀ ▀▀▀▀ ▀█▄▀▪ ▀▀▀ ▀▀▀
@skftn / dsc
@Lafudoci
@gingeropolous
@connorw600
Traceback (most recent call last):
File "C:\Users\walass\Desktop\moneriote-python-refactor\moneriote.py", line 55
2, in <module>
mon = Moneriote.from_config()
File "C:\Users\walass\Desktop\moneriote-python-refactor\moneriote.py", line 28
4, in from_config
return cls(**{**cfg_cloudflare, **cfg_monero})
TypeError: __init__() missing 4 required positional arguments: 'cf_domain_name',
'cf_subdomain_name', 'cf_dns_api_key', and 'cf_dns_api_email'
It's from the last commit. I believe the issue is on my side (windows x64). But I can't figure it out. Could you help with this?
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.
You are definitely on an older commit because the banner is still in the old style. New one: link. Try to pull from skftn/moneriote-python:refactor
Perhaps try from scratch:
git clone https://github.com/skftn/moneriote-python.git
cd moneriote-python
git checkout refactor
Or the equivalent on Windows ^^
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.
Tried again and the commit is the same, but I found the behavior is different between cmd command python moneriote.py
and moneriote
. The former runs root dir's moneriote.py (with old banner) instead of utils.py (new banner). I guess I should use the latter as README metioned. But then I got error about ModuleNotFoundError: No modual named 'rsa'.
After I pip install rsa, I got:
ModuleNotFoundError: No modual named 'suds'.
When I try pip install suds, I got:
ModuleNotFoundError: No modual named 'client'.
After pip install client and try to run moneriote --from-con fig config.ini
Then I got:
(venv) C:\Users\LeftC\Downloads\moneriote-python>moneriote --from-config config.ini
Traceback (most recent call last):
File "C:\Users\LeftC\Downloads\moneriote-python\venv\Scripts\moneriote-script.py", line 11, in <module> load_entry_point('moneriote', 'console_scripts', 'moneriote')()
File "C:\Users\LeftC\Downloads\moneriote-python\venv\lib\site-packages\click-7.0-py3.6.egg\click\core.py", line 764, in __call__ return self.main(*args, **kwargs)
File "C:\Users\LeftC\Downloads\moneriote-python\venv\lib\site-packages\click-7.0-py3.6.egg\click\core.py", line 717, in main rv = self.invoke(ctx)
File "C:\Users\LeftC\Downloads\moneriote-python\venv\lib\site-packages\click-7.0-py3.6.egg\click\core.py", line 956, in invoke return ctx.invoke(self.callback, **ctx.params)
File "C:\Users\LeftC\Downloads\moneriote-python\venv\lib\site-packages\click-7.0-py3.6.egg\click\core.py", line 555, in invoke return callback(*args, **kwargs)
File "c:\users\leftc\downloads\moneriote-python\moneriote\main.py", line 31, in cli from moneriote.moneriote import Moneriote
File "c:\users\leftc\downloads\moneriote-python\moneriote\moneriote.py", line 12, in <module> from moneriote.dns import DnsProvider
File "c:\users\leftc\downloads\moneriote-python\moneriote\dns\__init__.py", line 28, in <module> from moneriote.dns.transip import TransIP
File "c:\users\leftc\downloads\moneriote-python\moneriote\dns\transip.py", line 8, in <module> from suds.client import Client as SudsClient
File "C:\Users\LeftC\Downloads\moneriote-python\venv\lib\site-packages\suds\client.py", line 242
except Exception, e:
^
SyntaxError: invalid syntax
Please help :(
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.
First of all, thanks for trying :)
I think this will fix it:
- Git pull (I made a commit)
- Have your
config.ini
look like this:
[MoneroDaemon]
path = /home/dsc/Downloads/monero-gui-v0.12.3.0/monerod
address = 127.0.0.1
port = 18081
auth =
height_discovery_method = compare
[DNS]
provider = cloudflare
domain_name = example.com
subdomain_name = node
api_key = xxx
api_email = xxx
max_records = 5
With your details of course.
moneriote --from-config config.ini
(inside virtual env)
It should now work :)
@skftn , it's really nice and big work! I'll merge it (and maybe tag it as a new pre-release version?) after I got it run smoothly on my machine. But before a longer testing, my opennode scanner service will stick with old script. |
Sure, tag it as pre-release and meanwhile @erciccione can migrate it to the monero-ecosystem. |
Yep, we can start the transfer after this PR get merged :) |
@skftn , I could make it start from last commit. But there is stll something looks weird :( .
|
ure assuming xmrchain is working .. well , looks like it is. you guys should really improve this so it checks block hashes instead of just the height. luckily remote nodes have sort of flown under the radar, but it'd be so easy to make this network completely unusable by creating fake nodes that report the right height but then are just dead zombies. |
well, then again, you could make zomby nodes that just get the right hash from a regular node.... sybil sybil sybil everywhere |
@Lafudoci Could try try latest commit, using I supect something is wrong with the communication with monerod on Windows, specifically this is weird: @Gingeropolous There are indeed some ways to fuck with this system, it is however ultimately up to the client (monero-gui) to verify the integrity of the remote nodes. |
moneriote/moneriote.py
Outdated
|
||
try: | ||
resp = requests.get(url, timeout=2) | ||
assert resp.status_code == 404 |
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.
@skftn , xmrchain ref was nicely fixed by last commit. And I tried both w/ and w/o auth. I found node with auth returns 401 here instead of 404 then shows not reachable.
But node w/o auth still gave 0 peer and didn't figure out yet:
Update: peer issue see below.
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.
I tried assert resp.status_code == 404 or resp.status_code == 401
and it works well on my auth node.
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.
Fixed it in my last commit. Thanks :)
moneriote/moneriote.py
Outdated
# build proc args | ||
args = [ | ||
'--rpc-bind-ip', self.md_daemon_addr, | ||
'--rpc-bind-port', str(18081), |
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.
@skftn , I finally found reason here, my port setting in config.ini is 18083. I tried str(18083) and it finally start to load peers from rpc! But I don't understand why using self.md_daemon_port
will cause error 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.
You are correct. Sorry, I will fix :)
moneriote/moneriote.py
Outdated
|
||
# remove old records | ||
for i, node in enumerate(dns_nodes): | ||
if node.address not in nodes or i >= self.dns_provider.max_records: |
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.
Hmm.. seems not, but I don't understand why my final records are always more than 5.
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.
Perhaps that clause should just be if node.address not in nodes
. Ill verify when I get home.
@Lafudoci Could you try latest commit? |
@skftn , we are almost there, it ran well over lastnight. Last commit works as expected, but could you please also include the mentioned fix of auth node responds status_code 401 above? |
@Lafudoci done |
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.
@skftn, thank you, it's really a good job!
@Lafudoci You can now transfer this repo to monero-ecosystem. Let me know when done please |
@erciccione done! |
config.ini
, such asmax_records
for Cloudflaremonerod
child-process.zone_id
, so no zone id required inconfig.ini
.Moneriote()
class programmaticallyWould be nice if you could try this out and let me know what you think.