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

rpc: resetrootcache #451

Merged
merged 2 commits into from Aug 4, 2020
Merged

Conversation

tynes
Copy link
Contributor

@tynes tynes commented May 14, 2020

When playing with my node, I found that resetting the root cache was sometimes helpful. This adds a node RPC method called resetrootcache and it drops the root server cache completely. It would also be technically possible to expose deleting a single name (or a set of names) out of the cache.

DNS servers employ Dynamic DNS to allow for remote clients to configure the zone, so maybe it makes sense to have expose an RPC interface for the DNS servers. This would give users a more familiar interface for the configuration.

Or maybe restarting the node to clear the cache is good enough.

Copy link
Member

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

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

ACK 837ecb0

Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 837ecb056c2af8a9aa0bf555282be398755c401e
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAl7Cj2wACgkQ5+KYS2KJ
yTrOaBAAhvNrj3aEvbkMzp7FPZcM4DpNC74nGs2dsf6eYcE3c8bW8dbIgsr+GjcD
E7fdbTY4iZ/8uXYrbv0Kk0mTIHFdLyjR8Yy0VNPJdQuPvdBcjm8v82Nc+53Aoqw3
693QktICPZLlHojOt7WN+vtAfvXc5pPSVy+akvmiPjv/mjeOz6rewuBlk96b3mqu
VgrNoZnR0l3nYh8oNpjuZ+7h82Xgdo4ULiDAL0sdUgeeOF4fCQqHo9iAOUnckSEk
hGuD8E72zfMKHQfUS1TBn01Rva1qsFpKMFIr/VHhxBItRR4WbdkEAIhvH2UjO7Qw
KNZqbJ4iIzGJ6LY+0lWkKxwnbrOQGK8RVgr6aNmNaYJ79MiHRAyR/DwLtTVqD/2k
OHTdWuhir9pt6AjCmyPEu4hRsHU1rDjalG5EvIaFPChSQW8WnqPkzcrkBmxrXK62
+U3/38Y9LzHagfPwChKn9vyyDmNzhXED1eQ0ZpFsKabs5J4qqePuWOlKjMNhP36G
dQP69zUa9nIN8oqs8VVzIsmOCAepwoDRmSN14VOF1lbv+4Sfb3Ss8g8cUXX2tuDE
V03bBFtog3FmBjlcTd1XqF4X+250MqMP9pI02RDAsls+oU3IZid0Y+WgA8cT4RXo
ItBSd5XxhnOZ06VVXEb3QklJmFS+qU2FDjtxVi3DJuNuxr3JtPA=
=80pD
-----END PGP SIGNATURE-----

pinheadmz's public key is on keybase

@pinheadmz
Copy link
Member

This is cool and helpful thank you.

Test process:

  • Start hsd in regtest clean
  • dig @127.0.0.1 -p 25349 test-ns (null response, SOA)
  • run regtest-names script
  • dig @127.0.0.1 -p 25349 test-ns (SAME null response, from cache)
  • rpc resetrootcache
  • dig @127.0.0.1 -p 25349 test-ns (results in updated data NS record, etc)

@pinheadmz
Copy link
Member

@tynes wanna add a description to the CHANGELOG and we can land this one I think?

@tynes
Copy link
Contributor Author

tynes commented Jul 27, 2020

@pinheadmz Added a section to the CHANGELOG.md noting the resetrootcache RPC method's functionality.

@pinheadmz
Copy link
Member

I thought of one other thing here, this is the first time in node rpc that some other module is accessed directly. In the future if we have a setting for hsd that disables the DNS servers (or even a hack like kyokan/bob-wallet#140 (comment)) then this rpc will throw an error. Waddaya think? Maybe at least wrap the call to node.ns in a try/catch with an RPCError?

@tynes
Copy link
Contributor Author

tynes commented Jul 31, 2020

Good thinking @pinheadmz, I agree that there should be a way to start the node without the ns and rs. It seems to me that it would be easiest to do so by adding a new file to lib/node that doesn't import the ns and rs but does everything else in FullNode. This makes maintenance a bit more difficult, especially since there is already the SPVNode. We could also have a flag like --no-resolvers or something like that.

@pinheadmz
Copy link
Member

Ok great, this test still works, and the rpc command fails gracefully if DNS is removed from hsd. Incidentally, the rs.open() and ns.open() should probably also have if (this.ns) etc checks as well, but that is future work. I think this is ok to merge - any last thoughts @tynes ?

Copy link
Member

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

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

RE-ACK cdc73fd

Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK cdc73fdfc9aef25c641501e7eb8bc1cd5ffd158c
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAl8oGU4ACgkQ5+KYS2KJ
yTq76w/+OCWUlW6Wc9sOCNvWRBLCMN+iy5NvIgndYEsjvlntbLQehltZQW09ReAJ
60OMmWbnn+bh14N1tn4ULzUDJzVxhWXpVkGigCUvFiLj+hYTULJvsyXbotHUAPZU
pvKyBg03vR2I/nF+Y2cFj6a5infOu7wGtnb8SSOSexR3DHU8OWvlHRk+zAc/TgyR
mv/6igO6DbL0+vZITECFtRrWZ4nfQfnENEvPFUehEx3n+Uj8CLlnT6UfPPZa5gzR
atHEd1MXET7oWURA0+eeB9RjEzI9xeebFB9nHPNrHIVlSsqpYNKSPy/tv1Kq++XI
ZkApMFVSHtQUF5CrUE3QP7t55/rj4+LaG5DZ+R6rGpogT2N617q8cpndajbZXlCT
bd9KCIo/C49HApQjJ2mEoEzjcAhRLu/h0q4+q+3sCWvWKSUcSYf3s8MIy8g3lMR5
FiYP13U9Q3aDVZdMdybQR7lEchA8HcIPsd8piO05QJm9dgDvIOdhUjobdH7yW9WN
cneFVGfCdmugPQdUytYyg4S4rKZskwXho4b8NGK2YRrsoSeUeucZ54nwH9fFB9mv
u/C+eSbgtWqorgXGkPRZlDDOm/PEFbnY+QkoZZQcidLalX+8QiDQ4qunnExP9Xr0
aPsoKiDIjZThAs8IN4MLT51c3BtUqk7Rob/ANBrGbS0szzgIpTg=
=WZXb
-----END PGP SIGNATURE-----

pinheadmz's public key is on keybase

@tynes
Copy link
Contributor Author

tynes commented Aug 3, 2020

It does assume that the ns has a cache object present on it, maybe it would be better to add a method to the ns itself called resetCache?

@pinheadmz
Copy link
Member

good point. I think that would match the surrounding style as well.

@tynes
Copy link
Contributor Author

tynes commented Aug 3, 2020

@pinheadmz Pushed a new commit and squashed

@pinheadmz
Copy link
Member

Ok still LGTM. If you get a chance to add this (and also validateresource) to the API docs, that would be 🍨 🍰 🍕

I can fix changelog conflicts when we merge.

Copy link
Member

@pinheadmz pinheadmz left a comment

Choose a reason for hiding this comment

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

RE-RE-ACK c649d9e

Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK c649d9ede65997492b89a026ca44cef729531835
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAl8pckMACgkQ5+KYS2KJ
yTpLgA//WvyJtA2IjE+CTDHSBrQ8jLHL+op7IuCsGP8W8o2ttuqREvpK5gyIWEIX
93RRrRyg/ZTVQtuI1fJdMCH6MV1Kb2iT3xC+Kq0sNYYY9GzOOhL8i79aO9HTg4WH
oZGcM68NRVIyoi6hWqPokr5VBKx7dH9GgZ1HMyDIVd0ak1gUL/6Fnoo4Y8jAsBtR
guOLmfvBou7j1iVGB+fqYkUQ5tYrv1Xxkx/PmwNwjixr7iaXsVn8F+9eHByush1b
Cv0pWfW/N4VrYbtEum8jre8CTXZAQQBAu49k3vOD5jyuNOlbOR8NIVHF8q77TDJR
lJ8/CU0A61RUaVmVTb5zee0PgY8ysUDYT3yCravGA/pFFfDjYo85c05FnSMlqURu
RSvQExpbQJhL7aThT8rOXKsXgqKljo4VvQNLqVoJiqP4brdb1gFDD6SiP2qbbGx2
LPZUwmhF2P1Jwzxh04ThdTjxgJbh0n6NQX2zPFqvYToMXjPgvwdckTEt9jAY+9eb
p3gmaBip+vEGcj51/8KuTaFr2NH7GfCXQKuq8HSh4xB2nAjtBRV1hoJpuj1m5MeE
5aJZa0LMXrqelRWZPKURyFpuJSI21hi/LSkuAcg/5KPAIGZQZnw4QkK0cEGZAtuU
YQEBbGxaPeZAJESBGZNTytoQ8Wq6xAx1KJiPabCbUvBwTA0+pVg=
=K9x5
-----END PGP SIGNATURE-----

pinheadmz's public key is on keybase

@pinheadmz pinheadmz merged commit 940f463 into handshake-org:master Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants