Network id validation stream and forwarding #4921
Conversation
shortthefomo
changed the title
…to validate network_id is present in validation and ledger streams
|
@intelliot this one is ready for review |
| auto const view = m_ledgerMaster.getCurrentLedger(); | ||
|
|
||
| if (view->rules().enabled(featureNetworkIDValidation) && | ||
| val->getNetworkID() != static_cast<Json::UInt>(*networkID)) |
There was a problem hiding this comment.
This code is probably wrong. The cast is unnecessary and confusing but (if my understanding is correct) the call val->getNetworkID() which calls getFieldU32 which calls STObject::getFieldByValue will throw an exception if the sfNetworkID field is not present in the received validation.
There was a problem hiding this comment.
This is my first attempted commit to rippled and still trying to pick up C, so also learning lots as I go here. That being said will, test and validate this and possibly also add a test case to prove that we get the desired result here.
| return setup_.networkID; | ||
| if (auto const netid = setup_.networkID) | ||
| return netid; | ||
| return 0; |
There was a problem hiding this comment.
Previously this code could return std::nullopt but now it will return an std::optional with a value of 0 which (if I understand the code correctly) is the ID of the mainnet. That makes no sense and seems very wrong. Why even have the optional return?
There was a problem hiding this comment.
This is by design, we always want to default to mainnet if something goes wrong looking up the network ID.
There was a problem hiding this comment.
I think that this will result in a change of the behavior and I am not sure it is correct. The code previously could return std::nullopt and now won't and maybe some parts of the code relied on that? This change should be signed off by someone with better understanding of the codebase than me before you put the change in and admins of servers shoudl be informed.
| std::uint32_t | ||
| STValidation::getNetworkID() const | ||
| { | ||
| return getFieldU32(sfNetworkID); |
There was a problem hiding this comment.
I think you should return optional here. Something like:
std::optional<std::uint32_t>
STValidation::getNetworkID() const
{
if (!isFieldPresent(sfNetworkID))
return std::nullopt;
return getFieldU32(sfNetworkID);
}
There may be a way to do this more efficiently using the weird ~sfNetworkID syntax but I'm still learning code.
High Level Overview of Change
Context of Change
Rework of #4869 and add the feature request by @intelliot in #4783
Type of Change
API Impact