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 dumpzone: dump HNS root zone to file #534

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pinheadmz
Copy link
Member

@pinheadmz pinheadmz commented Jan 8, 2021

Closes #152

File output is standard zone file format: https://tools.ietf.org/html/rfc1035#section-5.1

Replaces #280
Based on https://github.com/james-stevens/handshake-bridge by @james-stevens

Major differences from these two predecessors:

  • Reads from the Urkel Tree, not the txn (which include record updates between the 36-block commit cycle)
  • Includes TXT records (handshake-bridge didn't either by design or by accident by calling Resource.toDNS() which results in a referral-only answer)

@pinheadmz pinheadmz requested a review from tynes January 8, 2021 20:55
@pinheadmz
Copy link
Member Author

sample:

melanie. 21600 IN NS ns1.melanie.
melanie. 21600 IN NS ns1.melanie.
xn--u86c8154b. 21600 IN NS ns1.xn--u86c8154b.
xn--u86c8154b. 21600 IN NS ns1.xn--u86c8154b.
cybermage. 21600 IN NS ns1.cybermage.
cybermage. 21600 IN NS ns1.cybermage.
proofofconcept. 21600 IN NS ns.proofofconcept.
proofofconcept. 21600 IN TXT "A simple social network for Handshake"
proofofconcept. 21600 IN TXT "support: hs1qde7jaw6qgzzfu83upn3twvsyhh0zrshg76qe0x"
proofofconcept. 21600 IN DS 17552 8 2 BBBE70AF5CD965360442CBEF40E3299344AF493D339592B93DAA29F7 839C1D58  ; alg = RSASHA256 ; hash = SHA256
chostner. 21600 IN NS ns1.chostner.
chostner. 21600 IN NS ns1.chostner.
n08. 21600 IN NS ns1.n08.
n08. 21600 IN NS ns1.n08.
nongcun. 21600 IN NS ns1.nongcun.
nongcun. 21600 IN NS ns1.nongcun.
xn--mgbkl3fffp. 21600 IN TXT "dummy"
xn--h1alfddcd4f2a. 21600 IN TXT "dummy"

const iter = tree.iterator(true);

let count = 0;
while ((await iter.next()) && (fd != null)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this fd check redundant with the check on 2571?

Copy link
Contributor

Choose a reason for hiding this comment

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

probably can remove one or other, just trying to catch if the fd.on('error',... got triggered

JS isn't my strongest skill, so likely all that error handling can all be done better
I think it was to try & catch things like disk-full

@coveralls
Copy link

coveralls commented Jan 8, 2021

Pull Request Test Coverage Report for Build 602759748

  • 5 of 47 (10.64%) changed or added relevant lines in 1 file are covered.
  • 1100 unchanged lines in 13 files lost coverage.
  • Overall coverage decreased (-0.07%) to 59.347%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/node/rpc.js 5 47 10.64%
Files with Coverage Reduction New Missed Lines %
lib/covenants/rules.js 1 68.63%
lib/protocol/consensus.js 1 82.79%
lib/net/netaddress.js 3 66.28%
lib/covenants/namestate.js 4 89.34%
lib/net/brontide.js 8 56.09%
lib/net/peer.js 12 33.59%
lib/net/hostlist.js 32 38.16%
lib/net/pool.js 34 30.34%
lib/node/fullnode.js 38 60.91%
lib/wallet/walletdb.js 128 77.88%
Totals Coverage Status
Change from base Build 377501416: -0.07%
Covered Lines: 19525
Relevant Lines: 30628

💛 - Coveralls

Co-authored-by: James Stevens <github@jrcs.net>
Co-authored-by: Mark Tyneway <mark.tyneway@gmail.com>
@tynes
Copy link
Contributor

tynes commented Jan 9, 2021

Handling a shutdown gracefully during this is also something to consider

@james-stevens
Copy link
Contributor

Excluding TXT wasn't intentional, I copied the code from #280, but if the name is a delegated subdomain, there should NOT be any TXT records in the parent zone for it anyway. An RFC DNS server would NEVER serve those records, they belong in the zone file for the sub-domain NOT the parent.

The parent owns the DS - and NS & any necessary IPv4 or IPv6 would be served, but all other records for a delegated subdomain would be ignored. named-checkzone will probably fail & moan to you about it.

So having these TXT and expecting them to be served & used is quite a big break of RFC 👎

If there are TXT in the zone file for the sub-domain (extremely common, e.g for email SPF records), how do you expect a TXT request to be handled?

  1. Give out ONLY the ones in the ROOT (breaks SPF)
  2. Give out ONLY those in the zone file (RFC)
  3. Merge both sets and give out that

By reading the data from the Urkel Tree, you probably solve a few race conditions if data is updated when the dump is run (:+1:), but it would also be nice if the SOA Serial given, when polled over UDP (say), reflected the Urkel Tree update-time, instead of just being the current date & time ... That way the zone data dumped would be consistent with the SOA Serial ... we did discuss this.

Using the Urkel Tree also means if you dump the zone on two different servers, within the same 36-block commit cycle, you should get the same zone - this is good & highly desirable for failover purposes 👍

@pinheadmz pinheadmz added this to the v2.4.0 milestone Jan 18, 2021
@pinheadmz
Copy link
Member Author

Added one more commit e80b835 which fixes another SYNTH4/SYNTH6 decoding issue that should have been caught in #444

Example output of my own mainnet name omnitude with SYNTH records:

omnitude. 21600 IN NS _5l6tm80._synth.
omnitude. 21600 IN DS 26614 8 2 A20BC6F9ADA0883326A05374D0D0C0E6290CEF580D00B9B957703014 41733B7F  ; alg = RSASHA256 ; hash = SHA256
omnitude. 21600 IN NS _400hjs000l2gol000fvvsc9cpg._synth.
_5l6tm80._synth. 21600 IN A 45.77.219.32
_400hjs000l2gol000fvvsc9cpg._synth. 21600 IN AAAA 2001:19f0:5:450c:5400:3ff:fe31:2ccc

Also worth noting that #566 may affect this rpc output concerning TXT records in the root zone, but I think we decided that was ok. Currently this branch will print TXT even if NS is present:

proofofconcept. 21600 IN NS ns.proofofconcept.
proofofconcept. 21600 IN TXT "A simple social network for Handshake"
proofofconcept. 21600 IN TXT "support: hs1qde7jaw6qgzzfu83upn3twvsyhh0zrshg76qe0x"
proofofconcept. 21600 IN DS 17552 8 2 BBBE70AF5CD965360442CBEF40E3299344AF493D339592B93DAA29F7 839C1D58  ; alg = RSASHA256 ; hash = SHA256
ns.proofofconcept. 21600 IN A 142.93.115.133

@nodech nodech added dns part of the codebase node-rpc part of the codebase tests part of the codebase labels Nov 18, 2021
@pinheadmz
Copy link
Member Author

pinheadmz commented Dec 9, 2021

new TODO: some names have unknown resource versions, bypass them, otherwise the process will throw and abort.

diff --git a/lib/node/rpc.js b/lib/node/rpc.js
index 1973aa98e..96f276574 100644
--- a/lib/node/rpc.js
+++ b/lib/node/rpc.js
@@ -2629,7 +2629,19 @@ class RPC extends RPCBase {
         continue;
 
       const fqdn = bns.util.fqdn(ns.name.toString('ascii'));
-      const resource = Resource.decode(ns.data);
+
+      let resource;
+      try {
+        resource = Resource.decode(ns.data);
+      } catch (e) {
+        this.logger.warning(
+          'could not process resource for name: %s (%s)',
+          fqdn,
+          e.message
+        );
+        continue;
+      }
+
       const zone = resource.toZone(fqdn);
       for (const record of zone) {
         if (fd == null)

result:

[debug] (node-rpc) dumpzone names processed: 2230000
[debug] (node-rpc) dumpzone names processed: 2240000
[warning] (node-rpc) could not process resource for name: roivantsciences. (Unknown serialization version: 2.)

@pinheadmz pinheadmz removed this from the v2.5.0 milestone Feb 8, 2022
continue;

const fqdn = bns.util.fqdn(ns.name.toString('ascii'));
const resource = Resource.decode(ns.data);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const resource = Resource.decode(ns.data);
try {
const resource = Resource.decode(ns.data);
} catch (e) {
continue;
}

Copy link

Choose a reason for hiding this comment

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

Per our testing this needs to be updated to:

let resource;
try {
resource = Resource.decode(ns.data);
if (!resource) continue;
} catch (e) {
continue;
}

@james-stevens
Copy link
Contributor

TBH: I think buffrr's AXFR plug-in is a better solution than this, so (personally) I'd just drop this

https://github.com/buffrr/hsd-axfr

Copy link

@nglabs42 nglabs42 left a comment

Choose a reason for hiding this comment

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

Merged with local pull of master, with rithvikvibhu's update at line 2581 of lib/node/rpc.js:
let resource;
try {
resource = Resource.decode(ns.data);
if (!resource) continue;
} catch (e) {
continue;
}
I was able to pull the zonefile, 16 GB.

continue;

const fqdn = bns.util.fqdn(ns.name.toString('ascii'));
const resource = Resource.decode(ns.data);
Copy link

Choose a reason for hiding this comment

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

Per our testing this needs to be updated to:

let resource;
try {
resource = Resource.decode(ns.data);
if (!resource) continue;
} catch (e) {
continue;
}

Copy link

@nglabs42 nglabs42 left a comment

Choose a reason for hiding this comment

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

tested RPC DUMPZONE

Copy link

@nglabs42 nglabs42 left a comment

Choose a reason for hiding this comment

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

Update lib/node/rpc.js
Line 2581 'const resource = Resource.decode(ns.data);'
to

let resource;
try {
resource = Resource.decode(ns.data);
if (!resource) continue;
} catch (e) {
continue;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns part of the codebase node-rpc part of the codebase tests part of the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dumpzone RPC Call
7 participants