-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Proposed 1.10.0-b1 #4270
Proposed 1.10.0-b1 #4270
Conversation
Trustlines must be between two different accounts but two trustlines exist where an account extends trust to itself. They were created in the early days, likely because of bugs that have been fixed. The new fixTrustLinesToSelf amendment will remove those trustlines when it activates.
We profiled different algorithms and data structures to understand which strategy is best from a performance standpoint: - Linear search on an array; - Binary search on a sorted array; - Using `std::map`; and - Using `std::unordered_map`. Both linear search and std::unordered_map outperformed the other alternatives so no change to the existing data structure is justified. If more handers are added, this should be revisited. For some additional details and timings, please see: #3298 (comment)
Co-authored-by: Chenna Keshava B S <ckbs.keshava56@gmail.com>
Each node on the network is supposed to have a unique cryptographic identity. Typically, this identity is generated randomly at startup and stored for later reuse in the (poorly named) file `wallet.db`. If the file is copied, it is possible for two nodes to share the same node identity. This is generally not desirable and existing servers will detect and reject connections to other servers that have the same key. This commit achives three things: 1. It improves the detection code to pinpoint instances where two distinct servers with the same key connect with each other. In that case, servers will log an appropriate error and shut down pending intervention by the server's operator. 2. It makes it possible for server administrators to securely and easily generate new cryptographic identities for servers using the new `--newnodeid` command line arguments. When a server is started using this command, it will generate and save a random secure identity. 3. It makes it possible to configure the identity using a command line option, which makes it possible to derive it from data or parameters associated with the container or hardware where the instance is running by passing the `--nodeid` option, followed by a single argument identifying the infomation from which the node's identity is derived. For example, the following command will result in nodes with different hostnames having different node identities: `rippled --nodeid $HOSTNAME` The last option is particularly useful for automated cloud-based deployments that minimize the need for storing state and provide unique deployment identifiers. **Important note for server operators:** Depending on variables outside of the the control of this code, such as operating system version or configuration, permissions, and more, it may be possible for other users or programs to be able to access the command line arguments of other processes on the system. If you are operating in a shared environment, you should avoid using this option, preferring instead to use the `[node_seed]` option in the configuration file, and use permissions to limit exposure of the node seed. A user who gains access to the value used to derive the node's unique identity could impersonate that node. The commit also updates the minimum supported server protocol version to `XRPL/2.1`, which has been supported since version 1.5.0 and eliminates support for `XPRL/2.0`.
Caching the base58check encoded version of an `AccountID` has performance advantages, because because of the computationally heavy cost associated with the conversion, which requires the application of SHA-256 twice. This commit makes the cache significantly more efficient in terms of memory used: it eliminates the map, using a vector with a size that is determined by the configured size of the node, and a hash function to directly map any given `AccountID` to a specific slot in the cache; the eviction policy is simple: in case of collision the existing entry is removed and replaced with the new data. Previously, use of the cache was optional and required additional effort by the programmer. Now the cache is automatic and does not require any additional work or information. The new cache also utilizes a 64-way spinlock, to help reduce any contention that the pressure on the cache would impose.
When starting, the code generates a new ephemeral private key and a corresponding certificate to go along with it. This process can take time and, while this is unlikely to matter for normal server operations, it can have a significant impact for unit testing and development. Profiling data suggests that ~20% of the time needed for a unit test run can be attributed to this. This commit does several things: 1. It restructures the code so that a new self-signed certificate and its corresponding private key are only initialized once at startup; this has minimal impact on the operation of a regular server. 2. It provides new default DH parameters. This doesn't impact the security of the connection, but those who compile from scratch can generate new parameters if they so choose. 3. It properly sets the version number in the certificate, fixing issue #4007; thanks to @donovanhide for the report. 4. It uses SHA-256 instead of SHA-1 as the hash algorithm for the certificate and adds some X.509 extensions as well as a random 128-bit serial number. 5. It rounds the certificate's "start of validity" period so that the server's precise startup time cannot be easily deduced and limits the validity period to two years, down from ten years. 6. It removes some CBC-based ciphers from the default cipher list to avoid some potential security issues, such as CVE-2016-2107 and CVE-2013-0169.
if (cmdline.count("newnodeid") != 0) | ||
clearNodeIdentity(*db); |
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.
Should there be some kind of error if --newnodeid
is specified in combination with a fixed node id (e.g. --nodeid
or [node_seed]
)?
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 don't think there's a problem with doing that. --newnodeid
clears any node identities stored in the database, but it may be worth notifying. We ought to fix this separately, IMO.
{ | ||
std::lock_guard lock(sl); | ||
cache_[index].id = id; | ||
std::strcpy(cache_[index].encoding, ret.c_str()); |
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 know there's an assert
a couple of lines up, but strcpy
always raises my hackles. Would it hurt to use strncpy
?
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 wouldn't hurt. I'll make a follow-up fix for this, if you don't mind merging this 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.
Tested in Windows with VS2019.
If merged this PR will:
fixTrustLinesToSelf
: Introduce amendment to handle trustlines to self #4105 (with fix for return value added!)