Replace charge() by fee_.update() in OnMessage functions

[bthomee]

High Level Overview of Change

In PeerImpl.cpp, if the function is a message handler (onMessage) or called directly from a message handler, then it should use fee_, since when the handler returns (OnMessageEnd) then the charge function is called. If the function is not a message handler, such as a job queue item, it should remain charge.

Context of Change

The current implementation can lead to double-charging.

Type of Change

• Bug fix (non-breaking change which fixes an issue)

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 21 lines in your changes missing coverage. Please review.

Project coverage is 78.1%. Comparing base (fa5a854) to head (4dc11a3).

Files with missing lines	Patch %	Lines
src/xrpld/overlay/detail/PeerImp.cpp	0.0%	19 Missing ⚠️
src/libxrpl/resource/Charge.cpp	0.0%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5269     +/-   ##
=========================================
- Coverage     78.2%   78.1%   -0.0%     
=========================================
  Files          790     790             
  Lines        67638   67645      +7     
  Branches      8164    8166      +2     
=========================================
+ Hits         52860   52864      +4     
- Misses       14778   14781      +3     

Files with missing lines	Coverage Δ
src/libxrpl/resource/Charge.cpp	71.4% <0.0%> (-7.5%)	⬇️
src/xrpld/overlay/detail/PeerImp.cpp	3.8% <0.0%> (-<0.1%)	⬇️

... and 4 files with indirect coverage changes

[bthomee]

@ximinez FYI: unrelated to this change, the fee_.update assert can be violated in void PeerImp::onMessage(std::shared_ptr<protocol::TMGetObjectByHash> const& m), where the fee is set to a moderate charge (=250) on line 2414 and can be updated to a malformed request charge (=200) when the ledger hash is invalid.

Either we change this fee or change our logic - both @Bronek and I suggested taking the max instead.

[bthomee]

@ximinez This replacement is in a for-loop. Instead of accumulated charges, now only a single invalid fee will be charged.

[vlntb]

This might fix some of the double-charges that we were detecting!

[ximinez]

@ximinez This replacement is in a for-loop. Instead of accumulated charges, now only a single invalid fee will be charged.

I would change this one back to a charge, and leave a comment explaining that multiple instances of invalid data should be charged / punished multiple times.

Another option is to change the continue to a break (or return, but break is better, because there may have been some good ones before the bad one(s)) with the idea that if any part of the message is corrupted, the whole message is probably bad, so don't even bother.

[bthomee]

I changed this to charge the fee for each malformed endpoint, while still allowing the valid endpoints to be processed.

@ximinez in which situations can an endpoint become invalid? If a peer at some endpoint disconnects and thus the message cannot be forwarded to it, that would seem like an innocent problem and it would be fine to charge the fee once as a warning / informational notice. If the message contains many invalid endpoints then we'd now charge more (as was the case before), which eventually can lead to being disconnected for bad behavior.

However, if the idea is that when any endpoint is invalid it means that the message is corrupted, then we should just return early and penalize the peer heavily, instead of still processing the first few valid endpoints (while ignoring any other valid endpoints after the invalid one).

[ximinez]

My understanding here is that "malformed" fee is charged if the string value fails to parse into an endpoint object (IP & port?). Whether the IP is available is not checked. So the charge doesn't fire if the source gives us "1.2.3.4:51234", but there's no rippled running there, but it will fire if the source gives us "your:mama".

[vlntb]

I'm not sure what to do with 4 other cases, where charge is called on the peer.
Does the mean that we are still double-charging in those cases? Once is from direct charge call, another from onMessageEnd?

i.e.

void
PeerImp::onMessage(std::shared_ptr<protocol::TMReplayDeltaRequest> const& m)
{
    JLOG(p_journal_.trace()) << "onMessage, TMReplayDeltaRequest";
    if (!ledgerReplayEnabled_)
    {
        fee_.update(
            Resource::feeMalformedRequest, "replay_delta_request disabled");
        return;
    }

    fee_.fee = Resource::feeModerateBurdenPeer;
    std::weak_ptr<PeerImp> weak = shared_from_this();
    app_.getJobQueue().addJob(
        jtREPLAY_REQ, "recvReplayDeltaRequest", [weak, m]() {
            if (auto peer = weak.lock())
            {
                auto reply =
                    peer->ledgerReplayMsgHandler_.processReplayDeltaRequest(m);
                if (reply.has_error())
                {
                    if (reply.error() == protocol::TMReplyError::reBAD_REQUEST)
                        peer->charge(
                            Resource::feeMalformedRequest,
                            "replay_delta_request");
                    else
                        peer->charge(
                            Resource::feeRequestNoReply,
                            "replay_delta_request");
                }

[ximinez]

@ximinez FYI: unrelated to this change, the fee_.update assert can be violated in void PeerImp::onMessage(std::shared_ptr<protocol::TMGetObjectByHash> const& m), where the fee is set to a moderate charge (=250) on line 2414 and can be updated to a malformed request charge (=200) when the ledger hash is invalid.

Either we change this fee or change our logic - both @Bronek and I suggested taking the max instead.

Simpler fix: Move the line setting the moderate charge to after the check that sets the malformed request charge. That check returns if it fails, so you don't have to worry about any other conflict.

[ximinez]

I'm not sure what to do with 4 other cases, where charge is called on the peer. Does the mean that we are still double-charging in those cases? Once is from direct charge call, another from onMessageEnd?

In the included example, the charge is called from a job queue thread. fee_ is no longer accessible (or more specifically is meaningless) from there.

I don't think all double charges are bad or wrong. This is a perfect example. If the job is added to the queue, then we are going to charge the default trivial (1) amount. That's the charge to put the item into the job queue. If, in that job, we discover that the request is bad, we penalize the peer appropriately. Note that if the request is not bad, no additional charges are called. I'm sure there are other jobs where charges are made from the job queue for good data, but I still think those are appropriate under the idea that we charge a trivial amount to get into the queue to do the real work.

Another possibility, that I thought of while writing this, is that we can

1. Create a feeFree that charges 0
2. In message handlers that spawn a job, set fee_.fee = feeFree
3. In the spawned job, ensure that all code paths call charge at least once, where the good path charges the trivial fee.

I can't say I actually like this option, though. First, because a trivial fee is so close to 0 it may as well be 0 with a well-behaved peer. Second, because if we create this option, there's always the chance, however slim, that someone may figure out how to exploit it to flood messages without getting disconnected.

Edit: I've already thought of an exploit: Overload the server with "free" messages that cause it to do a lot of work in a job. If you can get enough queued up, you may be able to cause resource exhaustion. By the time they run, and start charging the malicious peer, the damage is done. Also, even after the peer gets disconnected, all those jobs still have to be cleared out. They won't do work because they use weak ptrs, but that's still work the server has to do. At least with a trivial fee, there's some limit to how many messages a malicious peer can send.

[bthomee]

I'm not sure what to do with 4 other cases, where charge is called on the peer. Does the mean that we are still double-charging in those cases? Once is from direct charge call, another from onMessageEnd?

In the included example, the charge is called from a job queue thread. fee_ is no longer accessible (or more specifically is meaningless) from there.

I don't think all double charges are bad or wrong. This is a perfect example. If the job is added to the queue, then we are going to charge the default trivial (1) amount. That's the charge to put the item into the job queue. If, in that job, we discover that the request is bad, we penalize the peer appropriately. Note that if the request is not bad, no additional charges are called. I'm sure there are other jobs where charges are made from the job queue for good data, but I still think those are appropriate under the idea that we charge a trivial amount to get into the queue to do the real work.

Another possibility, that I thought of while writing this, is that we can

1. Create a feeFree that charges 0
2. In message handlers that spawn a job, set fee_.fee = feeFree
3. In the spawned job, ensure that all code paths call charge at least once, where the good path charges the trivial fee.

I can't say I actually like this option, though. First, because a trivial fee is so close to 0 it may as well be 0 with a well-behaved peer. Second, because if we create this option, there's always the chance, however slim, that someone may figure out how to exploit it to flood messages without getting disconnected.

Edit: I've already thought of an exploit: Overload the server with "free" messages that cause it to do a lot of work in a job. If you can get enough queued up, you may be able to cause resource exhaustion. By the time they run, and start charging the malicious peer, the damage is done. Also, even after the peer gets disconnected, all those jobs still have to be cleared out. They won't do work because they use weak ptrs, but that's still work the server has to do. At least with a trivial fee, there's some limit to how many messages a malicious peer can send.

Yes, let's definitely keep charging at least some fee.