XRPLF / rippled

Decentralized cryptocurrency blockchain daemon implementing the XRP Ledger protocol in C++
https://xrpl.org
ISC License
4.51k stars 1.46k forks source link

AccountTx reported ledger ranges #2327

Open sublimator opened 6 years ago

sublimator commented 6 years ago

Is it safe to honor the ledger ranges returned by account_tx these days? I vaguely recall an issue and Chris Clark (RippleAPI author) working around it by using a RangeSet implementation and looking at the reported ledger ranges:

https://github.com/ripple/ripple-lib/blob/develop/src/ledger/transactions.ts#L119 https://github.com/ripple/ripple-lib/blob/develop/src/common/connection.ts#L382 https://github.com/ripple/ripple-lib/blob/develop/src/common/connection.ts#L44 https://github.com/ripple/ripple-lib/blob/develop/src/common/rangeset.ts#L54

What's going on here? I checked, and at a quick glance it seems to report only validated ledger ranges, but I'm not confident in a "quick glance": https://github.com/ripple/rippled/blob/develop/src/ripple/app/ledger/impl/LedgerMaster.cpp#L384

Was rippled updated since that ripple-lib code was written? Was the issue unreported? Is the issue still there?

I'm helping someone with ripple, that isn't using RippleAPI, and it's nagging in the back of my head, but I don't have the time to dive into it myself.

sublimator commented 6 years ago

Looking a little more, it seems like the response intends to return the validated ledger range (sub)set of the requested range in the responses. Is this correct? By my reading of the docs (https://ripple.com/build/rippled-apis/#account-tx) that seems to be the case.

So I guess my question is, is it working as intended? I'd assume so at this point, after all these years. The ripple-lib code doesn't seem to look at the ledger_index_min/ledger_index_max in the response and instead looks at the validated_ledgers range set.

It's safe to assume there is no gaps in the account_tx ledger_index_min/ledger_index_max response range?

It's just that I also vaguely recall one of the integration guys saying this was an issue.

carlhua commented 4 years ago

@mDuo13 can you take a look at this to see if its purely a documentation or do we need to work this?

mDuo13 commented 4 years ago

@carlhua I do not know and I can't read the C++ code with enough confidence to find out.