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

Propose 1.9.1-b1 #4158

Closed
wants to merge 14 commits into from
Closed

Propose 1.9.1-b1 #4158

wants to merge 14 commits into from

Conversation

manojsdoshi
Copy link
Contributor

If merged this PR will:

close #4155
close #4152
close #4150
close #4149
close #4139
close #4134
close #4111
close #4097
close #4021
close #3965

This commit addresses minor bugs introduced with commit
6faaa91:

- The number of threads used by the database engine was
  incorrectly clamped to the lower possible value, such
  that the database was effectively operating in single
  threaded mode.

- The number of requests to extract at once was so high
  that it could result in increased latency. The bundle
  size is now limited to 4 and can be adjusted by a new
  configuration option `rq_bundle` in the `[node_db]`
  stanza. This is an advanced tunable and adjusting it
  should not be needed.
Several hard-coded parameters control the behavior of the ledger
acquisition engine. The values of many of these parameters where
set by intuition and have complex and non-intuitive interactions
with each other and other parts of the code.

An earlier commit attempted to adjust several of these parameters
to improve syncing performance; initial testing was promising but
a number of operators reported experiencing syncing and stability
issues with their servers. As a result, this commit reverts parts
of commit 1823506.

This commit further adjusts some tunables so as to increase the
aggressiveness of the ledger acquisition engine.
@MarkusTeufelberger
Copy link
Collaborator

Please don't introduce amendments in patch versions...

https://semver.org/

@ximinez
Copy link
Collaborator

ximinez commented May 10, 2022

Please don't introduce amendments in patch versions...

Partial agreement. ExpandedSignerList introduces a significant functionality change, so it may be better to wait for a minor version. However, fixNFTokenDirV1 is a bug fix for functionality introduced in 1.9.0 that hasn't been activated yet, and should be released ASAP.

Counterpoint: releasing the amendment doesn't activate the functionality. That's up to the community. If the amendment is included in this version, and the community decides to wait for the next release to vote for ExpandedSignerList, then nodes running 1.9.1 will not be amendment blocked, which is a significant upside. Note that both features have DefaultVote::No.

@MarkusTeufelberger
Copy link
Collaborator

I still changes the API in a backwards-compatible way. Just call it 1.10.0, numbers are cheap and plentiful. :-)

@nbougalis
Copy link
Contributor

nbougalis commented May 10, 2022

Please don't introduce amendments in patch versions...

https://semver.org/

Fair point @MarkusTeufelberger and something to think about, certainly but there are are some caveats.

I hope that you agree that the "fix" amendment for XLS-20 which is important and belongs in this patch release.

The other amendment probably doesn't belong into a "point" or patch release. I don't feel strongly enough to ask that it be removed from the 1.9.1 proposed release. If you (or others!) do, please speak up. Worst case scenario, we remove it from 1.9.1-b1 and, after 1.9.1 is released, we introduce a proposed 1.10.0-b1 that adds it.

@MarkusTeufelberger
Copy link
Collaborator

I see this rather pragmatically, a release is a release and unfortunately there is usually far less focus on making sure version numbers are meaningful in this space in general to rely on some real long term support or compatibility (imagine e.g. rippled 1.9.0 making it into Ubuntu LTS 22.04 - I'm skeptical that it will still be able to sync after the 5-9 years of support run out (https://endoflife.date/ubuntu). I'ts not like as if there are several branches from the past that are still maintained.

It was just an observation, no "omg, the sky is falling" kinda scenario. :-)

nbougalis and others added 12 commits May 10, 2022 13:34
One of the two versions of the `rngfill` function accepts a pointer
to a buffer and a size (in bytes). The function aims to fill the
provided `buffer` with `size` random bytes. It does this in chunks
of 8 bytes, for long as possible, and then fills any left-over gap
one byte at a time.

To avoid an annoying and incorrect warning about a potential buffer
overflow in the "trailing write", commit 78bc272
used a `#pragma` to instruct the compiler to not generate the incorrect
diagnostic. Unfortunately, this change _also_ eliminated the trailing
write code, which means that, under some cases, the `rngfill` function
would generate between 1 and 7 fewer random bytes than requested.

This problem would only manifest on builds that do not define `__GNUC__`
which, as of this writing, means MSVC.
* "A path is considered invalid if and only if it enters and exits an
  address node through trust lines where No Ripple has been enabled for
  that address." (https://xrpl.org/rippling.html#specifics)
* When loading trust lines for an account "Alice" which was reached
  via a trust line that has the No Ripple flag set on Alice's side, do
  not use or cache any of Alice's trust lines which have the No Ripple
  flag set on Alice's side. For typical "end-user" accounts, this will
  return no trust lines.
The amendment increases the maximum sign of an account's signer
list from 8 to 32.

Like all new features, the associated amendment is configured with
a default vote of "no" and server operators will have to vote for
it explicitly if they believe it is useful.
o Fixes an off-by-one when determining which NFTokenPage an
  NFToken belongs on.
o Improves handling of packed sets of 32 NFTs with
  identical low 96-bits.
o Fixes marker handling by the account_nfts RPC command.
o Tightens constraints of NFTokenPage invariant checks.

Adds unit tests to exercise the fixed cases as well as tests
for previously untested functionality.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants