Remove default constructor from SecretKey class #4607
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of sprintf use the suggested snprintf function
ckeshava
commented
Jul 3, 2023
…ta member localPubKey_. Use std::optional as a workaround for the same. Do not include PublicKey::emptyPubKey into the non publisherList_ associations
This does not modify the existing behavior of the function. Instead of modifying the PublicKey through a reference input parameter, the function is refactored to explicitly return the same PublicKey value. Reduces the obfuscation and improves readability of the code Remove the use of (void)<variable_name>; syntax. Use "_" to discard non-useful names. Refactor the constructor of Manifest class. Update the unit tests and internal code for the representation of revoked manifests - Include a PublicKey into ValidatorList::keyListings only if it is not empty - refactor ValidatorList::verify function and the use of optional PublicKeys
…into private - Update unit tests to use PublicKey::emptyKey. - Use easier implementation for RCLConsensus::Adaptor::validator(). Check for the presence of a non-empty PublicKey - Clean up comments: narrow down the questions/doubts about correct behavior of PublicKey::emptyPubKey - remove debug print statements
|
I've tried to remove the default construction of Please let me know if this looks okay. @seelabs This PR touches upon several aspects of the Manifest class. Whenever you are free, please take a look, thanks! |
…PublisherList will hold the ValidatorList information specified in the local configuration file. This data was previously stored in publisherList map. It was associated with the default-constructed PublicKey. This is an effort to remove such usages of the default-constructed Public Key.
…o be used to create zero-value initialized public keys. This was supposed to be a substitute for the default-constructed public keys.
Previous, this variable was built using the default constructor of the PublicKey class. Consequently, it's size_ == 0 and it would return true on an empty check. This commit aims to maintain an invariant: All constructed public keys are valid, non-empty and have 33 bytes of data. This commit is aimed at removing the size_ data member in the PublicKey class, thanks to the invariant stated above. New unit tests are introduced for PublicKey class. They perform the invariant checks on the PublicKey::emptyPubKey static data member
…onstructed public keys contain exactly 33 bytes of data, the size_ data member is rendered redundant. This commit optimizes the memory used per PublicKey object.
|
@nbougalis I was able to eliminate the |
|
@scottschurr Can you please take a look at this PR whenever you are free? Thanks! |
scottschurr
reviewed
Jul 29, 2023
…Keys in ValidatorKeys class into a single struct - introduce getter functions for reading these data members
scottschurr
suggested changes
Aug 3, 2023
There was a problem hiding this comment.
You got half of the point of my previous comment. But I was not clear enough about the motivation. Sorry about that.
Every time code is about to access to contents of an optional it needs to do one of two things:
- Have some prior knowledge so it "knows" that the
optionalhas valid contents, or - Verify, locally, that the
optionalhas a valid value prior to access.
If the code does not do one of those two things then that's a setup for a crash in the future.
The safest choice of those two, particularly in the face of an evolving code base, is to do the local check.
Putting all of the *Keys in ValidatorKeys into a single optional means that with one check...
if (validatorKeys_.keys)
I can be assured that it's safe to access any and all of the *Keys in ValidatorKeys::Keys.
If the *Keys are stored in three separate optionals, then I have to do separate checks any time I access any of the keys.
So, by putting all three *Keys in a single optional it becomes both
- Easy and
- Compute efficient
to see whether accessing any of the *Keys is safe.
Just in case I'm still not clear, I've made a commit where the optional is checked every time before the *Keys in ValidatorKeys are accessed. That's the top-most commit here: https://github.com/scottschurr/rippled/commits/chenna-ppp
You're welcome to cherry-pick that commit if you want. But don't blindly drop it in. Look at the places the checks are added. I might have messed something up.
Does this all make sense? From a larger perspective we're pushing first for safety and correctness. Once we have that, then we can make things efficient.
|
Okay, that clarifies things, thank you. I had my reservations about the
getter functions, I did not think about the downstream effects of an
unseated optional.
I'll make the necessary changes :+1
Regards,
Keshava
…On Thu, Aug 3, 2023 at 2:19 PM Scott Schurr ***@***.***> wrote:
***@***.**** requested changes on this pull request.
You got half of the point of my previous comment. But I was not clear
enough about the motivation. Sorry about that.
Every time code is about to access to contents of an optional it needs to
do one of two things:
- Have some prior knowledge so it "knows" that the optional has valid
contents, or
- Verify, locally, that the optional has a valid value prior to access.
If the code does not do one of those two things then that's a setup for a
crash in the future.
The safest choice of those two, particularly in the face of an evolving
code base, is to do the local check.
Putting all of the *Keys in ValidatorKeys into a single optional means
that with one check...
if (validatorKeys_.keys)
I can be assured that it's safe to access any and all of the *Keys in
ValidatorKeys::Keys.
If the *Keys are stored in three separate optionals, then I have to do
separate checks any time I access any of the keys.
So, by putting all three *Keys in a single optional it becomes both
1. Easy and
2. Compute efficient
to see whether accessing *any* of the *Keys is safe.
Just in case I'm still not clear, I've made a commit where the optional
is checked *every time* before the *Keys in ValidatorKeys are accessed.
That's the top-most commit here:
https://github.com/scottschurr/rippled/commits/chenna-ppp
<https://urldefense.com/v3/__https://github.com/scottschurr/rippled/commits/chenna-ppp__;!!PZTMFYE!5M7yWugj9lZGIdANUq_dIy6g1tt-tb9qQGmaXwWXl0guoI4Y-N89ukBKjLpSwGW4UkqJTTR1a7_CbTGAOcOokG_iuA$>
You're welcome to cherry-pick that commit if you want. But don't blindly
drop it in. Look at the places the checks are added. I might have messed
something up.
Does this all make sense? From a larger perspective we're pushing first
for safety and correctness. Once we have that, then we can make things
efficient.
------------------------------
In src/ripple/app/misc/impl/ValidatorKeys.cpp
<https://urldefense.com/v3/__https://github.com/XRPLF/rippled/pull/4607*discussion_r1283673587__;Iw!!PZTMFYE!5M7yWugj9lZGIdANUq_dIy6g1tt-tb9qQGmaXwWXl0guoI4Y-N89ukBKjLpSwGW4UkqJTTR1a7_CbTGAOcPUhHeNFg$>
:
> @@ -56,9 +56,7 @@ ValidatorKeys::ValidatorKeys(Config const& config, beast::Journal j)
}
else
{
- secretKey = token->validationSecret;
- publicKey = pk;
- masterPublicKey = m->masterKey;
+ keys.emplace(token->validationSecret, pk, m->masterKey);
Nice use of emplace()!
------------------------------
In src/ripple/app/misc/impl/ValidatorKeys.cpp
<https://urldefense.com/v3/__https://github.com/XRPLF/rippled/pull/4607*discussion_r1283681301__;Iw!!PZTMFYE!5M7yWugj9lZGIdANUq_dIy6g1tt-tb9qQGmaXwWXl0guoI4Y-N89ukBKjLpSwGW4UkqJTTR1a7_CbTGAOcO8h0vLAw$>
:
> @@ -83,10 +81,11 @@ ValidatorKeys::ValidatorKeys(Config const& config, beast::Journal j)
}
else
{
- secretKey = generateSecretKey(KeyType::secp256k1, *seed);
- publicKey = derivePublicKey(KeyType::secp256k1, *secretKey);
- masterPublicKey = *publicKey;
- nodeID = calcNodeID(*publicKey);
+ SecretKey sk = generateSecretKey(KeyType::secp256k1, *seed);
+ PublicKey pk = derivePublicKey(KeyType::secp256k1, sk);
+ keys.emplace(sk,
Nice use of emplace()!
------------------------------
In src/ripple/app/misc/ValidatorKeys.h
<https://urldefense.com/v3/__https://github.com/XRPLF/rippled/pull/4607*discussion_r1283684623__;Iw!!PZTMFYE!5M7yWugj9lZGIdANUq_dIy6g1tt-tb9qQGmaXwWXl0guoI4Y-N89ukBKjLpSwGW4UkqJTTR1a7_CbTGAOcMjB4KzLQ$>
:
> + SecretKey const& secret_,
+ PublicKey const& public_,
+ PublicKey const& masterPublic_
Please have the order of your constructor arguments match the order of the
public members in Keys. Putting them in a different order only promotes
confusion.
------------------------------
In src/ripple/app/misc/impl/ValidatorKeys.cpp
<https://urldefense.com/v3/__https://github.com/XRPLF/rippled/pull/4607*discussion_r1283688029__;Iw!!PZTMFYE!5M7yWugj9lZGIdANUq_dIy6g1tt-tb9qQGmaXwWXl0guoI4Y-N89ukBKjLpSwGW4UkqJTTR1a7_CbTGAOcNdLicbeg$>
:
> @@ -83,10 +81,11 @@ ValidatorKeys::ValidatorKeys(Config const& config, beast::Journal j)
}
else
{
- secretKey = generateSecretKey(KeyType::secp256k1, *seed);
- publicKey = derivePublicKey(KeyType::secp256k1, *secretKey);
- masterPublicKey = *publicKey;
- nodeID = calcNodeID(*publicKey);
+ SecretKey sk = generateSecretKey(KeyType::secp256k1, *seed);
+ PublicKey pk = derivePublicKey(KeyType::secp256k1, sk);
+ keys.emplace(sk,
+ derivePublicKey(KeyType::secp256k1, sk), pk);
Notice that you are calling derivePublicKey() with the same arguments
twice. Since the arguments are the same the two calls return identical
results. I think, compute time wise, you're better off if you make the
emplace() call like this:
keys.emplace(sk, pk, pk);
------------------------------
In src/ripple/app/consensus/RCLConsensus.cpp
<https://urldefense.com/v3/__https://github.com/XRPLF/rippled/pull/4607*discussion_r1283705757__;Iw!!PZTMFYE!5M7yWugj9lZGIdANUq_dIy6g1tt-tb9qQGmaXwWXl0guoI4Y-N89ukBKjLpSwGW4UkqJTTR1a7_CbTGAOcN4EZtMIw$>
:
> @@ -105,13 +105,13 @@ RCLConsensus::Adaptor::Adaptor(
JLOG(j_.info()) << "Validator identity: "
<< toBase58(
TokenType::NodePublic,
- *validatorKeys_.masterPublicKey);
+ *validatorKeys_.getMasterPublicKey());
The optional returning getter functions really miss the point of why the
three keys were gathered together into a single optional. If I'm not
confident that an optional I'm about to dereference is unseated I need to
check that optional -- does it have a value or not?
This function is not making that check. But it *could* make that check by
replacing the code on line 101 with...
if (validatorKeys_.keys)
If you make that single change they you can be confident that every time
you dereference any of the keys in validatorKeys_.keys there is a valid
key to access.
With the added getter function I'm back to not knowing whether the
returned value is useable. I should check the return value every time.
So I suggest that you remove the getter functions for two reasons:
- They hide the fact that a single check will let me know that all (or
none) of the keys are valid.
- Notice that ValidatorKeys has a bunch of public data members.
Embrace that. Go ahead and access the public data members as data members.
Does that make sense? It's possible I missed something. I'm open to being
wrong.
—
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/XRPLF/rippled/pull/4607*pullrequestreview-1561794763__;Iw!!PZTMFYE!5M7yWugj9lZGIdANUq_dIy6g1tt-tb9qQGmaXwWXl0guoI4Y-N89ukBKjLpSwGW4UkqJTTR1a7_CbTGAOcNoocg2rQ$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AFB4TNM3DILL4JHNDLOY7I3XTQIW3ANCNFSM6AAAAAAZ43VWPQ__;!!PZTMFYE!5M7yWugj9lZGIdANUq_dIy6g1tt-tb9qQGmaXwWXl0guoI4Y-N89ukBKjLpSwGW4UkqJTTR1a7_CbTGAOcOvNq7B8w$>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
… Use direct direct member access instead
ckeshava
commented
Aug 4, 2023
scottschurr
reviewed
Aug 9, 2023
|
Thank you very much, I'll work on this tomorrow
Regards,
Keshava
…On Wed, Aug 9, 2023 at 4:28 PM Scott Schurr ***@***.***> wrote:
***@***.**** commented on this pull request.
Your most recent changes regarding the optional in ValidatorKeys look
good to me. Nice work.
This is another partial review. I'm not done yet, but I wanted to get
these suggested changes to you before I leave for PTO.
------------------------------
In src/ripple/app/main/Application.cpp
<https://urldefense.com/v3/__https://github.com/XRPLF/rippled/pull/4607*discussion_r1286492055__;Iw!!PZTMFYE!5MegXq10PCh7U2wukkLXLgmRhYWWmYwwcNxX-32hxmC_0V5exwFibIAhFmMxSHsLohRbRzZFy_wqdGcDvvOpcSNKcw$>
:
> + // do not use the getter function from ValidatorKeys class because
+ // const& is returned from this function
I think the comment here no longer applies. The comment should probably be
removed.
------------------------------
In src/ripple/app/main/Application.cpp
<https://urldefense.com/v3/__https://github.com/XRPLF/rippled/pull/4607*discussion_r1289329172__;Iw!!PZTMFYE!5MegXq10PCh7U2wukkLXLgmRhYWWmYwwcNxX-32hxmC_0V5exwFibIAhFmMxSHsLohRbRzZFy_wqdGcDvvPVz-gwYQ$>
:
> @@ -584,7 +584,7 @@ class ApplicationImp : public Application, public BasicApp
return *m_jobQueue;
}
- std::pair<PublicKey, SecretKey> const&
+ std::optional<std::pair<PublicKey, SecretKey>> const&
nodeIdentity() override
Changing Application::nodeIdentity() to return an optional, is a problem
for all callers of nodeIdentity(). To safely use the new interface every
caller needs to check whether the optional is unseated before using it.
And, by the way, most of the places that were "fixed" to use the new
interface don't verify that the optional has a valid value. In my opinion
that's bad programming practice.
So I dug into the question. It turns out that the nodeIdentity is not
actually optional. It's always available shortly after the Application is
constructed. So the window where the nodeIdentity is unknown is very
short. And currently there are no callers of nodeIdentity() before the
value is known.
This leaves us in a place where we can say that calling nodeIdentity too
early in the boot process is a programming error. In fact, I think it's a
good idea to treat it like that.
With that in mind, I suggest that the nodeIdentity implementation be
changed to look like this:
std::pair<PublicKey, SecretKey> const&
nodeIdentity() override
{
if (nodeIdentity_)
return *nodeIdentity_;
LogicError(
"Accessing Application::nodeIdentity() before it is initialized.");
}
A LogicError has the effect of crashing the server with a good error
message. If the server is early in the boot process then crashing is often
a better choice that throwing an exception or just writing to the log.
Thoughts?
------------------------------
In src/ripple/app/consensus/RCLConsensus.cpp
<https://urldefense.com/v3/__https://github.com/XRPLF/rippled/pull/4607*discussion_r1289342420__;Iw!!PZTMFYE!5MegXq10PCh7U2wukkLXLgmRhYWWmYwwcNxX-32hxmC_0V5exwFibIAhFmMxSHsLohRbRzZFy_wqdGcDvvPv3HgG0Q$>
:
> @@ -799,10 +808,17 @@ RCLConsensus::Adaptor::validate(
validationTime = lastValidationTime_ + 1s;
lastValidationTime_ = validationTime;
+ if(!validatorKeys_.keys)
+ {
+ JLOG(j_.fatal()) << "RCLConsensus::Adaptor::validate: ValidatorKeys "
This should become the same logging level as is chosen on line 216.
------------------------------
In src/ripple/app/consensus/RCLConsensus.cpp
<https://urldefense.com/v3/__https://github.com/XRPLF/rippled/pull/4607*discussion_r1289342450__;Iw!!PZTMFYE!5MegXq10PCh7U2wukkLXLgmRhYWWmYwwcNxX-32hxmC_0V5exwFibIAhFmMxSHsLohRbRzZFy_wqdGcDvvNKI_Wjgg$>
:
> @@ -210,12 +210,21 @@ RCLConsensus::Adaptor::propose(RCLCxPeerPos::Proposal const& proposal)
proposal.prevLedger().begin(), proposal.prevLedger().size());
prop.set_proposeseq(proposal.proposeSeq());
prop.set_closetime(proposal.closeTime().time_since_epoch().count());
+
+ if(!validatorKeys_.keys)
+ {
+ JLOG(j_.fatal()) << "RCLConsensus::Adaptor::propose: ValidatorKeys "
I think that logging this as fatal() is overkill. I suggest error() or
even warn(). If a bug gets in that triggers this message then the logs
will be swamped with this log message. At fatal() level it is hard to
remove the message. And the recovery, simply returning, is benign.
—
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/XRPLF/rippled/pull/4607*pullrequestreview-1566313966__;Iw!!PZTMFYE!5MegXq10PCh7U2wukkLXLgmRhYWWmYwwcNxX-32hxmC_0V5exwFibIAhFmMxSHsLohRbRzZFy_wqdGcDvvPmGwCm4g$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AFB4TNODJSMPQ456V5VF7X3XUQMILANCNFSM6AAAAAAZ43VWPQ__;!!PZTMFYE!5MegXq10PCh7U2wukkLXLgmRhYWWmYwwcNxX-32hxmC_0V5exwFibIAhFmMxSHsLohRbRzZFy_wqdGcDvvNt90aLyg$>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
scottschurr
reviewed
Feb 6, 2024
Comment on lines
889
to
890
| // localPublisherList is always available. No need to perform the | ||
| // below check |
There was a problem hiding this comment.
Interesting. What happens if there is no [validators] section in the config file? If there's no information for a localPublisherList is it still available? It may be okay but, if so, it seems relevant to the comment. Thoughts?
Maybe more to the point, the comment says "No need to perform the below check." But if any of the publisherLists_ are not available then good is marked as false. Since the code works that way it seems like the check is dependent on the state of all of the publisherLists_, not just localPublisherList. Am I misunderstanding things?
|
@scottschurr No, you are correct in saying that all the The If the local configuration file does not specify a I hope this clears things up |
ximinez
requested changes
Feb 6, 2024
|
addressed all of your comments |
|
@scottschurr and/or @ckeshava : for feedback that has been addressed, feel free to use the I think |
scottschurr
approved these changes
Feb 28, 2024
Comment on lines
889
to
890
| // localPublisherList is always available. No need to perform the | ||
| // below check |
There was a problem hiding this comment.
@ckeshava, thoughts on this comment? I still think I would have understood your intent better with a comment like this:
// localPublisherList never expires, so localPublisherList is excluded
// from the below check.
But that quibble is not enough reason to delay this pull request further.
|
@scottschurr I have incorporated your suggestions for the comment in |
ximinez
approved these changes
Feb 29, 2024
ximinez
added
the
Passed
|
Suggested commit message: @ckeshava This PR has been through a lot of changes. Feel free to provide additional details that you think might be important. |
I'd add this line in the commit message ^^ @ximinez thanks for help, I couldn't have completed this PR without your contributions |
intelliot
added
the
Perf impact not expected
legleux
added a commit
to legleux/rippled
that referenced
this pull request
Apr 12, 2024
* Price Oracle (XLS-47d): (XRPLF#4789) (XRPLF#4789) Implement native support for Price Oracles. A Price Oracle is used to bring real-world data, such as market prices, onto the blockchain, enabling dApps to access and utilize information that resides outside the blockchain. Add Price Oracle functionality: - OracleSet: create or update the Oracle object - OracleDelete: delete the Oracle object To support this functionality add: - New RPC method, `get_aggregate_price`, to calculate aggregate price for a token pair of the specified oracles - `ltOracle` object The `ltOracle` object maintains: - Oracle Owner's account - Oracle's metadata - Up to ten token pairs with the scaled price - The last update time the token pairs were updated Add Oracle unit-tests * fix compile error on gcc 13: (XRPLF#4932) The compilation fails due to an issue in the initializer list of an optional argument, which holds a vector of pairs. The code compiles correctly on earlier gcc versions, but fails on gcc 13. * Set version to 2.2.0-b1 * Remove default ctors from SecretKey and PublicKey: (XRPLF#4607) * It is now an invariant that all constructed Public Keys are valid, non-empty and contain 33 bytes of data. * Additionally, the memory footprint of the PublicKey class is reduced. The size_ data member is declared as static. * Distinguish and identify the PublisherList retrieved from the local config file, versus the ones obtained from other validators. * Fixes XRPLF#2942 * Fast base58 codec: (XRPLF#4327) This algorithm is about an order of magnitude faster than the existing algorithm (about 10x faster for encoding and about 15x faster for decoding - including the double hash for the checksum). The algorithms use gcc's int128 (fast MS version will have to wait, in the meantime MS falls back to the slow code). * feat: add user version of `feature` RPC (XRPLF#4781) * uses same formatting as admin RPC * hides potentially sensitive data * build: add STCurrency.h to xrpl_core to fix clio build (XRPLF#4939) * Embed patched recipe for RocksDB 6.29.5 (XRPLF#4947) * fix: order book update variable swap: (XRPLF#4890) This is likely the result of a typo when the code was simplified. * Fix workflows (XRPLF#4948) The problem was `CONAN_USERNAME` environment variable, which Conan 1.x uses as the default user in package references. * Upgrade to xxhash 0.8.2 as a Conan requirement, enable SIMD hashing (XRPLF#4893) We are currently using old version 0.6.2 of `xxhash`, as a verbatim copy and paste of its header file `xxhash.h`. Switch to the more recent version 0.8.2. Since this version is in Conan Center (and properly protects its ABI by keeping the state object incomplete), add it as a Conan requirement. Switch to the SIMD instructions (in the new `XXH3` family) supported by the new version. * Update remaining actions (XRPLF#4949) Downgrade {upload,download}-artifact action to v3 because of unreliability with v4. * Install more public headers (XRPLF#4940) Fixes some mistakes in XRPLF#4885 * test: Env unit test RPC errors return a unique result: (XRPLF#4877) * telENV_RPC_FAILED is a new code, reserved exclusively for unit tests when RPC fails. This will make those types of errors distinct and easier to test for when expected and/or diagnose when not. * Output RPC command result when result is not expected. * Fix workflows (XRPLF#4951) - Update container for Doxygen workflow. Matches Linux workflow, with newer GLIBC version required by newer actions. - Fixes macOS workflow to install and configure Conan correctly. Still fails on tests, but that does not seem attributable to the workflow. * perf: improve `account_tx` SQL query: (XRPLF#4955) The witness server makes heavily use of the `account_tx` RPC command. Perf testing showed that the SQL query used by `account_tx` became unacceptably slow when the DB was large and there was a `marker` parameter. The plan for the query showed only indexed reads. This appears to be an issue with the internal SQLite optimizer. This patch rewrote the query to use `UNION` instead of `OR` and significantly improves performance. See RXI-896 and RIPD-1847 for more details. * `fixEmptyDID`: fix amendment to handle empty DID edge case: (XRPLF#4950) This amendment fixes an edge case where an empty DID object can be created. It adds an additional check to ensure that DIDs are non-empty when created, and returns a `tecEMPTY_DID` error if the DID would be empty. * Enforce no duplicate slots from incoming connections: (XRPLF#4944) We do not currently enforce that incoming peer connection does not have remote_endpoint which is already used (either by incoming or outgoing connection), hence already stored in slots_. If we happen to receive a connection from such a duplicate remote_endpoint, it will eventually result in a crash (when disconnecting) or weird behavior (when updating slot state), as a result of an apparently matching remote_endpoint in slots_ being used by a different connection. * Remove zaphod.alloy.ee hub from default server list: (XRPLF#4903) Remove the zaphod.alloy.ee hubs from the bootstrap and default configuration after 5 years. It has been an honor to run these servers, but it is now time for another entity to step into this role. The zaphod servers will be taken offline in a phased manner keeping all those who have peering arrangements informed. These would be the preferred attributes of a boostrap set of hubs: 1. Commitment to run the hubs for a minimum of 2 years 2. Highly available 3. Geographically dispersed 4. Secure and up to date 5. Committed to ensure that peering information is kept private * Write improved `forAllApiVersions` used in NetworkOPs (XRPLF#4833) * Don't reach consensus as quickly if no other proposals seen: (XRPLF#4763) This fixes a case where a peer can desync under a certain timing circumstance--if it reaches a certain point in consensus before it receives proposals. This was noticed under high transaction volumes. Namely, when we arrive at the point of deciding whether consensus is reached after minimum establish phase duration but before having received any proposals. This could be caused by finishing the previous round slightly faster and/or having some delay in receiving proposals. Existing behavior arrives at consensus immediately after the minimum establish duration with no proposals. This causes us to desync because we then close a non-validated ledger. The change in this PR causes us to wait for a configured threshold before making the decision to arrive at consensus with no proposals. This allows validators to catch up and for brief delays in receiving proposals to be absorbed. There should be no drawback since, with no proposals coming in, we needn't be in a huge rush to jump ahead. * fixXChainRewardRounding: round reward shares down: (XRPLF#4933) When calculating reward shares, the amount should always be rounded down. If the `fixUniversalNumber` amendment is not active, this works correctly. If it is not active, then the amount is incorrectly rounded up. This patch introduces an amendment so it will be rounded down. * Remove unused files * Remove packaging scripts * Consolidate external libraries * Simplify protobuf generation * Rename .hpp to .h * Format formerly .hpp files * Rewrite includes $ find src/ripple/ src/test/ -type f -exec sed -i 's:include\s*["<]ripple/\(.*\)\.h\(pp\)\?[">]:include <ripple/\1.h>:' {} + * Fix source lists * Add markers around source lists * fix: improper handling of large synthetic AMM offers: A large synthetic offer was not handled correctly in the payment engine. This patch fixes that issue and introduces a new invariant check while processing synthetic offers. * Set version to 2.1.1 * chore: change Github Action triggers for build/test jobs (XRPLF#4956) Github Actions for the build/test jobs (nix.yml, mac.yml, windows.yml) will only run on branches that build packages (develop, release, master), and branches with names starting with "ci/". This is intended as a compromise between disabling CI jobs on personal forks entirely, and having the jobs run as a free-for-all. Note that it will not affect PR jobs at all. * Address compiler warnings * Fix search for protoc * chore: Default validator-keys-tool to master branch: (XRPLF#4943) * master is the default branch for that project. There's no point in using develop. * Remove unused lambdas from MultiApiJson_test * fix Conan component reference typo * Set version to 2.2.0-b2 * bump version * 2.2.3 * 2.2.4 * 2.2.5 --------- Co-authored-by: Gregory Tsipenyuk <[email protected]> Co-authored-by: seelabs <[email protected]> Co-authored-by: Chenna Keshava B S <[email protected]> Co-authored-by: Mayukha Vadari <[email protected]> Co-authored-by: John Freeman <[email protected]> Co-authored-by: Bronek Kozicki <[email protected]> Co-authored-by: Ed Hennis <[email protected]> Co-authored-by: Olek <[email protected]> Co-authored-by: Alloy Networks <[email protected]> Co-authored-by: Mark Travis <[email protected]> Co-authored-by: Gregory Tsipenyuk <[email protected]>
sophiax851
pushed a commit
to sophiax851/rippled
that referenced
this pull request
Jun 12, 2024
* It is now an invariant that all constructed Public Keys are valid, non-empty and contain 33 bytes of data. * Additionally, the memory footprint of the PublicKey class is reduced. The size_ data member is declared as static. * Distinguish and identify the PublisherList retrieved from the local config file, versus the ones obtained from other validators. * Fixes XRPLF#2942
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
High Level Overview of Change
This Pull Request deletes the default constructor from SecretKey and PublicKey classes.
All the constructed Public Keys are valid, non-empty and contain 33 bytes of data. This PR establishes this invariant.
Additionally, the memory footprint of the
PublicKeyclass is reduced. Thesize_data member is declared asstatic.Context of Change
Fix #2942
Type of Change