Proposed 2.3.1-rc1: Reduce the peer charges for well-behaved peers #5243
Conversation
vlntb
changed the title
include/xrpl/resource/detail/Logic.h
Outdated
| assert( | ||
| feeDrop.cost() > feeInvalidSignature.cost() && | ||
| feeInvalidSignature.cost() > feeInvalidRequest.cost() && | ||
| feeInvalidRequest.cost() > 10); |
There was a problem hiding this comment.
As these fees are const, can you move these to a static initializer that's guaranteed to only be called once? Or otherwise something that isn't called as frequently, like a constructor?
I chatted with Ed and he suggested possibly make_Manager in ResourceManager.cpp? It's called from the Application ctor.
There was a problem hiding this comment.
I just had an alternate idea. Instead of re-using "convenient" existing variables, which have changed in this PR, define 3 new Charge values, specifically defined for use here.
// Only use these for logging in Logic::charge, and keep the values in descending order
Charge const feeLogAsWarn(3000, "log as warn");
Charge const feeLogAsInfo(1000, "log as info");
Charge const feeLogAsDebug(100, "log as debug");
And then we don't need an assert at all, because they won't change independently of their ordering in the logs.
src/libxrpl/resource/Fees.cpp
Outdated
| @@ -22,11 +22,11 @@ | |||
| namespace ripple { | |||
| namespace Resource { | |||
|
|
|||
| Charge const feeInvalidRequest(100, "malformed request"); | |||
| Charge const feeInvalidRequest(200, "malformed request"); | |||
There was a problem hiding this comment.
For some of the variables here and below, the name and description are inconsistent.
- invalid request vs. malformed request => invalid is more general while malformed is more specific.
- request no reply vs unsatisfiable request => no reply is more specific while unsatisfiable is more general.
- unwanted vs. useless => similar, but different. unwanted data can still be useful, just not wanted right now.
- etc.
Can you rename the variables or modify the descriptions to improve consistency?
There was a problem hiding this comment.
IMHO, changing the variable names would be better because the descriptions are used in log messages, which, while not a fixed API or anything, are more visible.
There was a problem hiding this comment.
That was my worry, too - we are relying on the logs a lot, and we need to be sure that we are not breaking someone's dashboard if we change the message in the area that hasn't been updated for a while. If we all agree, I'll be happy to update the variable names.
There was a problem hiding this comment.
Updating the variable names sounds good.
There was a problem hiding this comment.
There are a couple of discrepancies when updating the fee for received messages, whereby a mixture of calls to fee_.update and charge happen in the OnMessage.
The first comment provides more details. I only highlighted the charge calls that should be changed to fee_.update. There may be other calls in existing code that GitHub didn't show me, but which then also need to be modified. All this can be done in a follow-up PR, but it's so simple you may as well include it now.
include/xrpl/resource/detail/Logic.h
Outdated
| // Only use these for logging in Logic::charge, and keep the values in | ||
| // descending order | ||
| static Charge const feeLogAsWarn(3000, "log as warn"); | ||
| static Charge const feeLogAsInfo(1000, "log as info"); | ||
| static Charge const feeLogAsDebug(100, "log as debug"); |
There was a problem hiding this comment.
I would recommend defining these in Fees.h/cpp so the consequences of changing any of the fees are more apparent in terms of how they also will be logged.
There was a problem hiding this comment.
I actually had the opposite thought - now that I see them here, there's no need to define these as Charges. They can just be straight up ints. (Or more specifically, Charge::value_type to ensure the types remain consistent.)
e.g.
static constexpr Charge::value_type feeLogAsWarn = 3000;
Even if we change the values in Fees.cpp in the future, it wouldn't make sense to change them so radically that these cutoffs should ever need to change. e.g. There should always be something that only charges 1. Even the things we changed, we made the values larger, so if anything, they'll log at a higher level, which is probably what we want.
I think a comment in Fees.cpp would be sufficient.
// See also Resource::Logic::charge for log level cutoff values
Now, all that said, if you still disagree, I don't object to moving them to Fees.cpp 😄
Edit: And if you notice there, I used constexpr, so you could add a quick static_assert that won't hurt anything.
static_assert(feeLogAsWarn > feeLogAsInfo && feeLogAsInfo > feeLogAsDebug && feeLogAsDebug > 10);
There was a problem hiding this comment.
I'm not sure about this. Putting these definitions into Fees.h/cpp will encourage someone to use them as actual fees mistakenly. Keeping them close to the log-selection logic makes more sense to me.
include/xrpl/resource/detail/Logic.h
Outdated
| // Only use these for logging in Logic::charge, and keep the values in | ||
| // descending order | ||
| static Charge const feeLogAsWarn(3000, "log as warn"); | ||
| static Charge const feeLogAsInfo(1000, "log as info"); | ||
| static Charge const feeLogAsDebug(100, "log as debug"); |
There was a problem hiding this comment.
I actually had the opposite thought - now that I see them here, there's no need to define these as Charges. They can just be straight up ints. (Or more specifically, Charge::value_type to ensure the types remain consistent.)
e.g.
static constexpr Charge::value_type feeLogAsWarn = 3000;
Even if we change the values in Fees.cpp in the future, it wouldn't make sense to change them so radically that these cutoffs should ever need to change. e.g. There should always be something that only charges 1. Even the things we changed, we made the values larger, so if anything, they'll log at a higher level, which is probably what we want.
I think a comment in Fees.cpp would be sufficient.
// See also Resource::Logic::charge for log level cutoff values
Now, all that said, if you still disagree, I don't object to moving them to Fees.cpp 😄
Edit: And if you notice there, I used constexpr, so you could add a quick static_assert that won't hurt anything.
static_assert(feeLogAsWarn > feeLogAsInfo && feeLogAsInfo > feeLogAsDebug && feeLogAsDebug > 10);
| break; | ||
| case ListDisposition::invalid: | ||
| // This shouldn't ever happen with a well-behaved peer | ||
| fee_ = Resource::feeInvalidSignature; | ||
| fee_.update( | ||
| Resource::feeInvalidSignature, "invalid list disposition"); |
There was a problem hiding this comment.
Is invalid signature here appropriate? If not but we just want to charge the peer a lot, you can create a new const fee.
There was a problem hiding this comment.
This error is returned from
// The returned PublicKey value is read from the manifest. Manifests do not
// contain the default-constructed public keys
std::pair<ListDisposition, std::optional<PublicKey>>
ValidatorList::verify
from several places including cryptographic signature check.
I agree that we need to look into this and potentially add more granular error types here, because we may get this error for non-cryptographic reasons too, but I wouldn't suggest doing this as part of the hotfix.
| assert(f >= fee); | ||
| fee = f; |
There was a problem hiding this comment.
reading how we call fee_.update in PeerImp.cpp I do not see anything to provide the guarantee that this assert wants. Should this be fee = std::max(f, fee); instead ?
There was a problem hiding this comment.
... or even if (f < fee) return;
There was a problem hiding this comment.
handler.onMessageBegin(
header.message_type,
m,
header.payload_wire_size,
header.uncompressed_size,
header.algorithm != Algorithm::None);
handler.onMessage(m);
handler.onMessageEnd(header.message_type, m);
We always start from onMessageBegin, which sets the fee to the lowest level (feeTrivialPeer = 1).
We only process one message in this run.
This guarantees that fee update always goes from 1 to any other fee that is higher.
src/xrpld/overlay/detail/PeerImp.cpp
Outdated
| // Something new: Dynamic fee | ||
| fee_.fee = Resource::Charge(m->transactions_size(), "transactions"); |
There was a problem hiding this comment.
Given that the existing code does not have a corresponding fee for one or more transactions, what is the practical effect of this change?
The handleTransaction function is called for each transaction and already applies charges. Does charging this dynamic fee here cause double-charging or does it serve to fill a gap that currently exists?
There was a problem hiding this comment.
I agree. Introducing dynamic fees needs to be better thought through. We are introducing the potential of a fee going down, something that ChargeWithContext::update currently does not support. I propose to remove this dynamic fee change from the hotfix and return later once we have better unit-test coverage for this part of the logic.
There was a problem hiding this comment.
This function calls handleTransaction (along with onMessage(std::shared_ptr<protocol::TMTransaction>)).
handleTransaction could update the fee to feeUselessData, which has a value of 150. A TMTransactions message could potentially be a lot bigger than that.
Not only that, though, if the message is big enough (which is currently impossible with a well-behaved peer, but possible in the future), the dynamic fee could take the total pretty close to the dynamic fee level. Probably best to remove this and rethink it for the future.
src/xrpld/overlay/detail/PeerImp.cpp
Outdated
| JLOG(p_journal_.debug()) | ||
| << "Caching " << (batch ? "batch" : "unsolicited") | ||
| << " pseudo-transaction tx " << tx->getID(); |
There was a problem hiding this comment.
This log message doesn't seem to match what is happening here. There's canonicalization and possibly relaying going on, while the log message suggests that both batch and non-batch (unsolicited) tx are being cached.
I'd also recommend just having three separate and appropriately phrased log messages, depending on the situation below (skipped, relayed, charged).
Also, to confirm:
- One of the fixes introduced in this hotfix was to not relay pseudo-tx, but isn't that what actually can happen here anyway?
- If a pseudo-tx is non-batch it is unwanted - is there any point to canonicalize and possibly relay it then, before charging the peer for useless data? If possible, moving the
if (!batch)check up and then returning early would seem sensible.
There was a problem hiding this comment.
One of the fixes introduced in this hotfix was to not relay pseudo-tx, but isn't that what actually can happen here anyway?
Yes, ideally, we should be doing this check in a single place, but unfortunately, there are several other code paths that can lead to a relay call. So, the final decision about relaying is made inside of the relay call itself.
If a pseudo-tx is non-batch it is unwanted - is there any point to canonicalize and possibly relay it then, before charging the peer for useless data? If possible, moving the if (!batch) check up and then returning early would seem sensible.
I agree in principle. However,
canonicalize - not only streamlines the format but also adds to the cache, which we need.
relay - not only relays transactions but also adds transactions to the queue, which we also want to keep.
I would create a separate ticket to refactor relay, canonicalize and other method names that do not represent what is currently happening.
There was a problem hiding this comment.
Added extra debug logs
vlntb
force-pushed
the
fix/signaturefee-to-unwanteddata
branch
from
7dcfec9 to
00edaea
Compare
vlntb
added
the
Ready to merge
vlntb
force-pushed
the
fix/signaturefee-to-unwanteddata
branch
2 times, most recently
from
c0ff860 to
b755b01
Compare
- Fix an erroneous high fee penalty that peers could incur for sending
older transactions.
- Update to the fees charged for imposing a load on the server.
- Prevent the relaying of internal pseudo-transactions.
- Before: Pseudo-transactions received from a peer will fail the signature
check, even if they were requested (using TMGetObjectByHash), because
they have no signature. This causes the peer to be charge for an
invalid signature.
- After: Pseudo-transactions, are put into the global cache
(TransactionMaster) only. If the transaction is not part of
a TMTransactions batch, the peer is charged an unwanted data fee.
These fees will not be a problem in the normal course of operations,
but should dissuade peers from behaving badly by sending a bunch of
junk.
- Improve logging: include the reason for fees charged to a peer.
Co-authored-by: Ed Hennis <[email protected]>
ximinez
force-pushed
the
fix/signaturefee-to-unwanteddata
branch
from
b755b01 to
f3e201f
Compare
ximinez
changed the title
High Level Overview of Change
This is a hotfix release that includes the following updates:
Context of Change
Type of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)