-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Patch CTID #4738
base: develop
Are you sure you want to change the base?
Patch CTID #4738
Conversation
@intelliot I believe devent was reset to a networkID < 1024 therefore it did not have a ctid. @cindyyan317 can you please recheck the devnet again with your locally running PR? |
Hi @dangell7 . for the transaction "CCBBD2F5CEE90FA7A2ED13A7A343D015F57950B562185734E125B7BE740F2428", i still can not see its ctid from tx. However , i can see its ctid from account_tx output.
What does it mean? It should not have ctid when networdID < 1024? devnet's networkId has been always 2 , right? |
Yes, I think Devnet's NetworkID has always been @dangell7 , I think it should be possible (and would be ideal) to always return |
You're right and I will look at the issue deeper this weekend. I don't like peering from my work computer. I have an env now to spin up a peer easier to test. I will also check on the CTID on txs <= 1024. I think I misspoke. |
@intelliot moving this to draft while I write more tests. Can confirm ctid is returned when network id <= 1024. |
@scottschurr Thanks again for your update. I have fixed all of the issues. However, we are getting some weird behavior from this. Its sometimes on the tx and sometimes not. We are chasing this down now. I believe more tests need to be written for this or a bug needs to be fixed. As soon as we can replicate the issue I will fix this. |
Given
Could you Of course, when you do think it may be ready, please mark it as "ready for review". |
Internal tracker: RPFC-82 |
note: @dangell7 indicated he will push changes tomorrow. |
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #4738 +/- ##
=======================================
Coverage 71.0% 71.0%
=======================================
Files 796 796
Lines 66793 66820 +27
Branches 10987 10986 -1
=======================================
+ Hits 47420 47442 +22
- Misses 19373 19378 +5
|
|
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.
👍 Looks good to me.
@intelliot working on the codecov |
auto metaset = | ||
std::make_shared<TxMeta>(tr->getID(), tr->getLedger(), rawMeta); | ||
|
||
// if properly formed meta is available we can use it to generate ctid | ||
if (metaset->getAsObject().isFieldPresent(sfTransactionIndex)) |
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.
@scottschurr Can you look at this? Wouldn't sfTransactionIndex always exist? If I do account_tx
on a non validated ledger I get "Ledger not validated." SetStatus
values are optional. So can we just call this every time?
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.
Or how do I test account_tx
with a tx that doesnt have a sfTransactionIndex?
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.
@dangell7, thanks for asking. I can't immediately think of a situation where you could retrieve metadata for a transaction from a ledger and not have sfTransactionIndex
filled in. However I prefer the conservative approach of verifying that the field is present before accessing it. Bugs happen. Doing checks like this can prevent a bug elsewhere from turning catastrophic.
Taking a step back, we ran for a few years without code coverage as part of CI. And, thanks to some great work by good folks, we started getting code coverage reports again recently. But we're still learning how to best apply the tool. There will be code that can't be exercised by reasonable tests (testing asserts
anyone?). We have a new microscope and we've discovered that there are tiny mites living on our skin. Over time we'll learn to relax and know we don't have to check that each individual mite is harmless.
Having said that, if I were bound and determined to exercise the else
condition here, I'd doing it by calling the convertBlobsToTxResult()
function directly. That would allow me to pass in whatever I wanted in the rawMeta
argument. This test would pass in rawMeta
that omitted an sfTransactionIndex
. I'd guess that getting that test in place might take a day or so of effort.
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.
Awesome. Thank you for this response. I do like the code coverage stuff. I will take a look at how to get some more coverage on this. convertBlobsToTxResult
sounds pretty simple. Thanks
@dangell7 how would you like this to move forward? |
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.
👍 The code looks good to me. @dangell7 is this ready to merge or are there still things that need to be resolved?
@seelabs Yeah this is ready. I tried to test the |
|
I tried to build a rippled with this PR to run on the devnet's full history node, but since this PR was based off the 2.2.0-rc1, so the build is getting amendment blocked. @dangell7 can you please rebase off the master branch so I can build it for 2.2.0? Thanks. |
High Level Overview of Change
Fix a number of issues involved with CTID.
tx
transactions.ErrorCodes.cpp
Context of Change
Type of Change
.gitignore
, formatting, dropping support for older tooling)API Impact
libxrpl
change (any change that may affectlibxrpl
or dependents oflibxrpl
)