Pr/2.2.2 rc2 #5106
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -781,7 +781,7 @@ class NetworkOPsImp final : public NetworkOPs | |
|
|
||
| StateAccounting accounting_{}; | ||
|
|
||
| std::set<std::pair<PublicKey, uint256>> pendingValidations_; | ||
| std::mutex validationsMutex_; | ||
|
|
||
| private: | ||
|
|
@@ -2311,13 +2311,14 @@ NetworkOPsImp::recvValidation( | |
| << "recvValidation " << val->getLedgerHash() << " from " << source; | ||
|
|
||
| std::unique_lock lock(validationsMutex_); | ||
| if (pendingValidations_.contains( | ||
| {val->getSignerPublic(), val->getLedgerHash()})) | ||
| return false; | ||
| pendingValidations_.insert({val->getSignerPublic(), val->getLedgerHash()}); | ||
| lock.unlock(); | ||
| handleNewValidation(app_, val, source); | ||
|
There was a problem hiding this comment. @mtrippled, my understanding is this is meant to prevent later on the calls to Asking to understand if you instead considered change the call at this There was a problem hiding this comment. @bachase close. I want to prevent concurrent calls to checkAccept() for the same validation mainly because of the call to InboundLedgers::acquire(), which then calls InboundLedger::update(). InboundLedger::update() is where I've seen lock contention for duplicate. If it also stops redundantly sending out peer messages, that's fine, too. But the main goal is to not get stuck on locks. InboundLedger::update() gets stuck on a mutex unique to each inbound ledger. So, multiple calls to process the same inbound ledger at the same time get stuck if that mutex is locked. That's what saturates the job queue. In addition to avoiding the mutex on the individual InboundLedger object, this change also avoids the mutex on the shared InboundLedgers object. I have not seen it being contended, so I'm not sure it will help us, but I thought it would make sense to avoid using that lock also. Yes, I am trying to filter out redundant validations at the highest level once submitted to the job queue. I actually implemented these changes on 2 different days, each as a point solution. They work the same way, but the code paths are distinct. It didn't occur to me to use the same filtering function. |
||
| lock.lock(); | ||
| pendingValidations_.erase({val->getSignerPublic(), val->getLedgerHash()}); | ||
| lock.unlock(); | ||
|
|
||
| pubValidation(val); | ||
|
|
||
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.
Are there any other fields that might uniquely identify a validation that this would suppress? For example, the sequence number, or the cookie that might be important to detect faulty validators.
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.
@bachase I don't know. Also, I don't know how many validations saturating the job queue are duplicates from the same validator vs all the validations coming from distinct validators. In the latter case, none would be suppressed. I don't think the sequence number is useful if the hashes are distinct--also, I think some place else checks that the sequence number matches the ledger we're trying to validate. I don't know anything about the validation cookie either.