Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add logic to switch from bandwidth to rc_plugin for HF 20 #2665

Merged
merged 5 commits into from
Aug 6, 2018

Conversation

mvandeberg
Copy link
Contributor

@mvandeberg mvandeberg commented Jul 20, 2018

Closes #2648

@mvandeberg mvandeberg force-pushed the 2648-update-witness-for-rc branch from fc8ae22 to b84e063 Compare July 20, 2018 20:03
@theoreticalbts
Copy link
Contributor

Here are the requirements as I see them:

  • (a) It should be difficult / impossible to create a block producing configuration where both bandwidth algorithms are active simultaneously.
  • (b) It should be difficult / impossible to create a block producing configuration where no bandwidth algorithm is active and all transactions are accepted regardless of bandwidth.
  • (c) The default should be to use the old bandwidth system until HF20, then switch to the new bandwidth system.
  • (d) It should be possible to easily force using the old bandwidth system, regardless of the current HF.
  • (e) It should be possible to easily force using the new bandwidth system, regardless of the current HF.
  • (f) A block producing configuration should require maintaining both data structures.

Now let's audit the code against these requirements.

My understanding is that adding rc_plugin to APPBASE_PLUGIN_REQUIRES means that if the witness plugin is active, then the RC plugin must also be active. (If this macro has different semantics, it should probably be documented and renamed.)

This approach makes requirements (b) and (f) easy. Also, the code does carefully consider the default case, so it passes (c).

The main problem is requirement (a). The RC plugin is always active, it needs to have some kind of toggle flag as well. And due to requirement (a), it should refuse to produce blocks if neither flag is active.

Is requirement (a) really a requirement? Think it through. If it turns out that on the live network, the new code is too restrictive, we will need a way to turn it off. If it is restricting transactions, and there is no configuration toggle to turn it off, that would be a problem.

@mvandeberg
Copy link
Contributor Author

What needs to be added is a way to set skip_reject_not_enough_rc from the config. For whatever I assumed there was a way to do this, but upon further review that does not appear to be the case.

Additionally, if we want to prevent overly restrictive semantics, we can perform and xor between !rc_skip_reject_not_enough_rc and witness-enforce-bandwidth with the default being rc_ski_reject_not_enough_rc=false and witness_enforce_bandwidth=false when HF 20 activates.

@theoreticalbts
Copy link
Contributor

theoreticalbts commented Aug 3, 2018

The plugin_initialize check in witness_plugin, in pseudocode, is essentially:

witness_plugin._enforce_bandwidth != rc_plugin.skip_reject_not_enough_rc

Unfortunately, these two flags have opposite "polarity":

  • _enforce_bandwidth = true means the witness_plugin will check bandwidth
  • skip_reject_not_enough_rc = false means the rc_plugin will check bandwidth

The "proper" fix, architecturally, is probably to refactor so that both flags the same "polarity" [1].

The "quick hack" fix is to just change the inequality from != to ==, although it makes the code hard to follow (so a short comment explaining the situation would not be amiss).

My suggestion is to put the "quick hack" fix in this PR, then make a separate issue/PR for the flag rename / repolarization. (Edit: I made issue #2703)

[1] Since this code's never been in a numbered release, we should be able to just rename the config option without breaking anyone's deployments. We won't have this luxury once the code makes it into a numbered release.

@mvandeberg
Copy link
Contributor Author

The amount of work to get #2703 implemented over the quick fix was negligible. This PR now has the fix described in #2703 as well.

@theoreticalbts
Copy link
Contributor

Looks like it's ready to merge

@mvandeberg mvandeberg merged commit e6111bd into develop Aug 6, 2018
@mvandeberg mvandeberg deleted the 2648-update-witness-for-rc branch August 6, 2018 17:11
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants