Skip to content
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

Check amendment block status and update with ledgers: #2137

Closed
wants to merge 1 commit into from
Closed

Check amendment block status and update with ledgers: #2137

wants to merge 1 commit into from

Conversation

mellery451
Copy link
Contributor

Check and modify amendment blocked status with each new ledger (provided
by @wilsonianb). Honor blocked status in certain RPC commands and when
deciding whether to propose/validate.

Fixes: RIPD-1479
Fixes: RIPD-1447

Release Notes

This resolves an issue whereby an amendment blocked server would still
serve some RPC requests that are unreliable in blocked state and would
continue to publish validations.

@codecov-io
Copy link

codecov-io commented Jun 8, 2017

Codecov Report

Merging #2137 into develop will decrease coverage by 0.11%.
The diff coverage is 81.81%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2137      +/-   ##
===========================================
- Coverage    69.48%   69.37%   -0.12%     
===========================================
  Files          685      684       -1     
  Lines        50520    50486      -34     
===========================================
- Hits         35105    35023      -82     
- Misses       15415    15463      +48
Impacted Files Coverage Δ
src/ripple/app/misc/AmendmentTable.h 100% <ø> (ø) ⬆️
src/ripple/protocol/ErrorCodes.h 100% <ø> (ø) ⬆️
src/ripple/app/consensus/RCLConsensus.cpp 63.27% <100%> (-0.14%) ⬇️
src/ripple/protocol/impl/ErrorCodes.cpp 94.49% <100%> (+0.05%) ⬆️
src/ripple/rpc/impl/RPCHandler.cpp 63.75% <100%> (+1.9%) ⬆️
src/ripple/app/ledger/impl/LedgerMaster.cpp 46.83% <40%> (+3.16%) ⬆️
src/ripple/app/misc/impl/AmendmentTable.cpp 92.52% <88.88%> (-0.27%) ⬇️
src/ripple/net/RPCSub.h 0% <0%> (-100%) ⬇️
src/ripple/net/impl/RPCSub.cpp 1.29% <0%> (-36.37%) ⬇️
src/ripple/rpc/handlers/Subscribe.cpp 61.49% <0%> (-31.15%) ⬇️
... and 77 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87742a5...c6f3e6f. Read the comment docs.

Copy link
Collaborator

@bachase bachase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice work. Left some nits which are all optional. If you haven't already, it would be useful to see this live on a test network.

{
return rpcAMENDMENT_BLOCKED;
}

if (!context.app.config().standalone() &&
handler->condition_ & NEEDS_CURRENT_LEDGER)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the NEEDS_CLOSED_LEDGER also only work if not amendment blocked? Note that I don't see any RPCs actually using that condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I wondered that too and had a hard time deciding since, as you point out, there aren't any methods currently using it. I think I'll add it to the check as you suggest.

Copy link
Contributor

@wilsonianb wilsonianb Jun 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of using these conditions to determine what commands to reject when an amendment blocked.
At first, I was concerned about NO_CONDITION commands such as account_channels, account_objects, channel_authorize, channel_verify, and ledger_entry that are affected by amendments. But commands specific to an amendment would simply return an unknownCmd error, and when I reverted the commit that added PayChan, rippled crashed before I could even try looking up account objects/channels or ledger entries

Terminating thread doJob: OrderBookDB::update: unhandled St13runtime_error 'invalid ledger entry type'

All that to say, I think this change is a good addition to the existing limitations in rippled's ability to serve RPC requests while amendment blocked.

resp.body.find("connectivity is working.") != std::string::npos);

// mark the Network as Amendment Blocked, but still won't fail until
// ELB is enabled (next step)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had trouble finding where ELB_SUPPORT is documented. Based on this test, it seems to disable RPC calls once the server is blocked?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know anything about it either, but I'm guessing it's short for "elastic load balancer" support and the only impact in the code is to perform some checks in Application::serverOkay, which is invoked by requesting / on a websocket enabled port. I imagine this is how load balancers are supposed to health-check the backend rippled servers.

The idea in this test is that I'm making the GET request on the WS port and expecting to see if fail under the right conditions (ELB enabled and server amendment blocked). Not knowing the history, I don't understand why we can't just always return a meaningful status and ditch the ELB_SUPPORT config.

@@ -47,6 +47,8 @@ class AmendmentTable
virtual bool isEnabled (uint256 const& amendment) = 0;
virtual bool isSupported (uint256 const& amendment) = 0;

virtual bool isMissingSupport () = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a test to AmendmentTable_test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

@@ -157,6 +157,9 @@ class AmendmentTableImpl final
// we haven't participated in one yet.
std::unique_ptr <AmendmentSet> lastVote_;

// True if an unsupported amendment is enabled
bool missingSupport_;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider renaming this and the accessor method unsupportedEnabled. To me, missing support sounds like a potential amendment has yet to receive enough votes by the network and is therefore missing support.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missingSupport_ --> unsupportedEnabled?

@@ -755,6 +769,7 @@ class AmendmentTable_test final : public beast::unit_test::suite
testDetectMajority ();
testLostMajority ();
testSupportedAmendments ();
testHasUnsupported ();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this

{
return rpcAMENDMENT_BLOCKED;
}

if (!context.app.config().standalone() &&
handler->condition_ & NEEDS_CURRENT_LEDGER)
Copy link
Contributor

@wilsonianb wilsonianb Jun 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of using these conditions to determine what commands to reject when an amendment blocked.
At first, I was concerned about NO_CONDITION commands such as account_channels, account_objects, channel_authorize, channel_verify, and ledger_entry that are affected by amendments. But commands specific to an amendment would simply return an unknownCmd error, and when I reverted the commit that added PayChan, rippled crashed before I could even try looking up account objects/channels or ledger entries

Terminating thread doJob: OrderBookDB::update: unhandled St13runtime_error 'invalid ledger entry type'

All that to say, I think this change is a good addition to the existing limitations in rippled's ability to serve RPC requests while amendment blocked.

@bachase
Copy link
Collaborator

bachase commented Jun 12, 2017

👍 on the update too. Thanks for adding the test!

@@ -47,6 +47,14 @@ class AmendmentTable
virtual bool isEnabled (uint256 const& amendment) = 0;
virtual bool isSupported (uint256 const& amendment) = 0;

/**
* @brief returns true if one more more amendments on the network
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more more --> or more?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

@@ -157,6 +157,9 @@ class AmendmentTableImpl final
// we haven't participated in one yet.
std::unique_ptr <AmendmentSet> lastVote_;

// True if an unsupported amendment is enabled
bool missingSupport_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missingSupport_ --> unsupportedEnabled?

jr = env.rpc ("owner_info", alice.human()) [jss::result];
BEAST_EXPECT (jr.isMember(jss::accepted) && jr.isMember(jss::current));

// not tested (TODO): path_find, submit, submit_multisigned
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see tests for submit/submit_multisigned since the docs explicitly mention Cannot submit or process transactions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - will add something for those.

Copy link
Contributor

@wilsonianb wilsonianb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM
Thanks for crippling future outdated versions of rippled 😄

@mellery451 mellery451 added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 21, 2017
Check and modify amendment blocked status with each new ledger (provided
by @wilsonianb). Honor blocked status in certain RPC commands and when
deciding whether to propose/validate.

Fixes: RIPD-1479
Fixes: RIPD-1447

Release Notes
-------------

This resolves an issue whereby an amendment blocked server would still
serve some RPC requests that are unreliable in blocked state and would
continue to publish validations.
@seelabs
Copy link
Collaborator

seelabs commented Jul 20, 2017

In 0.80.0-b2

@seelabs seelabs closed this Jul 20, 2017
@mellery451 mellery451 deleted the amend-block branch August 8, 2017 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants