Support for name lookups in -connect and -addnode #192
Conversation
|
This is a recent rough draft, but it needs improvement. Think about the object structure a bit. You want a "CAddress factory", that will create one or more CAddress's from a DNS lookup. Supporting a multiple-address lookup from inside a singleton address object is awkward, because DNS lookups have a one-to-many relationship between DNS names and addresses. The "create higher nIndex, until address returned is invalid" is unusual for this reason, and the code should be refactored. You want a helper (CAddressFactory?) that returns one or more CAddress objects, from a DNS lookup. Because a DNS lookup may produce multiple addresses, this cannot be inside the CAddress implementation itself. |
|
I agree, actually. I moved the lookup code to a NameLookup function that creates a CAddress or a vector of those, and is used in the (single-address only) lookup version of the CAddress constructor, for convenience. |
|
ACK, thanks for revising |
|
Forum thread http://www.bitcoin.org/smf/index.php?topic=7123.0 |
|
My finger was hovering over the "Merge pull request" button, but I found a last minute issue. Eventually, we do want to support IPv6. Satoshi reserved space for it in the P2P protocol address storage area, and it will be needed in years to come, to connect some clients. As such, please separate hostname and port into two distinct function parameters / variables, and do not combine them into a single string "host:port", only to be parsed and separated in NameLookup() Rationale:
So, separate hostname and port, and you're golden with this commit. Thanks. |
|
I actually started writing this patch by using getaddrinfo, since its interface is much cleaner and supports seamless upgrading to IPv6. Not knowing what the compatibility of that is with other operating systems, i sticked to gethostbyname. Considering IPv6 hostnames, I would suggest first trying to parse the string passed as [host]:port, and if this parsing fails, resort to host:port. Otherwise we will still need some function somewhere to do that parsing anyway, if people want to pass an address including a port number on the command line or config file somewhere. |
* A new option -dns is introduced that enables name lookups in -connect and -addnode, which is not enabled by default, as it may be considered a security issue. * A Lookup function is added that supports retrieving one or more addresses based on a host name * CAddress constructors (optionally) support name lookups. * The different places in the source code that did name lookups are refactored to use NameLookup or CAddress instead (dns seeding, irc server lookup, getexternalip, ...). * Removed ToStringLog() from CAddress, and switched to ToString(), since it was empty.
|
Added string+port number constructors. |
|
ACK |
Support for name lookups in -connect and -addnode
Refine and update README
Issue bitcoin#192: For now nothing really uses this, so we are not going to complicate maintenance by renaming it just yet.
Issue bitcoin#192: For now nothing really uses this, so we are not going to complicate maintenance by renaming it just yet. cherry-picked from: ade0464
Do not check scripts unless blocks are less than 30 days old
Include amount into sighash, for offline and SPV wallet security.
Changed some copyright headers
Fixes for the qa tests related to the coinbase maturity
…bitcoin#192) - closing tx, shutdown tx, etc: to avoid any confusion I took the "shutdown tx" out and simply named it the "on-chain tx" - replaced "party" with "channel partner" for consistency - removed "as many people think". If the book does a good job, not many people will think that ! In any case, this phrase does not help. Better to just state the facts. - etc
-connect and -addnode, which is not enabled by default, as
it may be considered a security issue.
one or more addresses based on a host name
are refactored to use NameLookup or CAddress instead (dns seeding,
irc server lookup, getexternalip, ...).