sfcodes harmonization for hooks #4089
Conversation
|
@nbougalis Hi Nik, can you + team take a good look at this and consider merging? I think we can all agree Hooks are something we do not want to rush, but is definitely something serious enough to reserve codes. We want to prevent conflicts like happened with Tickets. |
RichardAH
force-pushed
the
sfcode_harmonization
branch
2 times, most recently
from
dcde02b to
3c8163f
Compare
|
@scottschurr and I will review with a goal of getting this into a 1.9.0-b1; we will rebase #4101 on top of this. |
Appreciate it. A lot :) |
|
See above referenced commit for just how annoying it can be to have your sfcodes clobbered. Reiterating that we need a better system going forward if multiple people are going to work simultaneously on amendments that change or add sfFields. |
|
@RichardAH, I respect the pain you have felt as new serialization types are added to the system while you're maintaining code. I think reserving the codes that Hooks needs is a good idea. I have two concerns with this commit as it currently stands:
Visibility of Field Usage Hooks adds quite a number of serialized fields, and that's fine. What's not so good is that we don't see these serialized fields being used anywhere in the code base as it stands. I get why that is. But it's not uncommon for me to look in the SFields.cpp file and wonder, "Hmmm, who is using that field?" With the newly added Hooks fields if I look in the code base my answer will be nobody. But that's not actually the case. They are used, by Hooks, but I can't see that usage. Additionally I am of the opinion that if something is not tested then it often doesn't work as expected. This commit adds Hooks fields, but does not exercise them in any way. That's probably okay? But it leaves me uneasy. I'd like to suggest that, in addition to adding the Hooks-specific SFields, you add a Hooks stub unit test file. It doesn't need to do anything other than:
This solves both problems. It identifies the facility that will eventually use these fields. It also does some minimal exercising of the newly added fields. Does that seem like a reasonable request? I'm open to other suggestions if you see other ways to address my concerns, Order of Operations As software engineers, we all respect that order of operations matters a lot. This pull request, as it is currently coded, would sit nicely atop the XLS20 pull request once that is merged to develop. However @nbougalis suggests that we should add this commit before XLS20 is merged. If we take that approach, then I'm uncomfortable with the pull request as it stands. If this commit is added before XLS20 is merged, then the pull request presciently adds fields that aren't needed until XLS20 puts them to use. In effect, if this commit is merged before XLS20 is merged, then I'd like this commit to be a Hooks Only commit. Let's remove from this commit the fields that XLS20 will introduce. I'll deal with that problem when I do the rebase of XLS20 to sit atop the 1.9.0 commits. Summary In summary, assuming we put this commit in before XLS20 is merged, I'd like to see:
Would it actually be easier for you if we merge this commit after XLS20 is merged? I'm not intending to make extra work for you (other than adding the unit tests). Thoughts? Thanks. |
RichardAH
force-pushed
the
sfcode_harmonization
branch
from
3c8163f to
bf791bc
Compare
RichardAH
changed the title
|
I have made the changes you have requested. |
There was a problem hiding this comment.
Thanks for making these changes. I think they will help with the maintenance of the code base, particularly when we look back through history.
I've left a few comments, none of which are critical. So I'm approving this pull request as it stands. But it would be nice to see the comments addressed. So I'm supplying a commit that you may use as an example of how the comments might be addressed. Or, if you prefer, you could simply cherry-pick the commit on top of your changes. You'll find the commit here: scottschurr@94f62f0
Many thanks for your on-going contributions to the XRP Ledger.
| @@ -891,6 +891,7 @@ if (tests) | |||
| src/test/protocol/InnerObjectFormats_test.cpp | |||
| src/test/protocol/Issue_test.cpp | |||
| src/test/protocol/KnownFormatToGRPC_test.cpp | |||
| src/test/protocol/Hooks_test.cpp | |||
There was a problem hiding this comment.
While not critical, it would be nice if this in alphabetical order like the other entries.
| extern SF_UINT64 const sfHookOn; | ||
| extern SF_UINT64 const sfHookInstructionCount; | ||
| extern SF_UINT64 const sfEmitBurden; | ||
| extern SF_UINT64 const sfHookReturnCode; |
There was a problem hiding this comment.
There's a distinction between the common and uncommon SFields. The common ones have a slightly more efficient encoding. I had thought that we were pretty consistent about calling out which are which in this file. But on closer inspection I see that we've not been consistent.
It would be nice to add those comments into this file in those places where they are missing. But, again, that's not critical.
| CONSTRUCT_TYPED_SFIELD(sfTicketSequence, "TicketSequence", UINT32, 41); | ||
| CONSTRUCT_TYPED_SFIELD(sfHookStateCount, "HookStateCount", UINT32, 45); |
There was a problem hiding this comment.
There are a number of places where this commit introduces gaps in the integer codes for SFields, like this jump from 41 to 45. I get where that's coming from. I also know that closing those gaps would be disruptive to maintenance of the hooks branch.
It might be nice to call out the newly introduced gaps with comments so future pull requests (like support for XLS20) can more easily fill in those gaps.
| **/ | ||
|
|
||
| void | ||
| testHookFields() |
There was a problem hiding this comment.
Thanks for adding this unit test. I think it will help with future maintenance.
High Level Overview of Change
Hooks and XLS20 both require new serialised field codes in rippled. These codes are currently in conflict.
Hooks has previously had a conflict with Tickets which has caused considerable confusion.
Our ecosystem does not currently have a system for assigning codes to new amendments. This needs to be addressed.
Allowing conflicts to continue is detrimental to third party tools, applications and wallets which need to serialise and deserialise rippled's data in order to function. These include but are not limited to the ripple-binary-codec and binary visualizer tool.
This PR is an attempt to resolve the current sfcode conflicts between XLS20 and Hooks. Other pending amendments are not included.
Context of Change
Type of Change