XRPLF / rippled

Decentralized cryptocurrency blockchain daemon implementing the XRP Ledger protocol in C++
https://xrpl.org
ISC License
4.51k stars 1.46k forks source link

Finish physical redesign #4997

Closed thejohnfreeman closed 3 months ago

thejohnfreeman commented 5 months ago

This is the final and most disruptive phase of the physical redesign. It makes these changes:

There is a non-trivial (but, I think, easy) process to merge existing branches-in-progress with this one. I will describe it here. Look at the sequence of commits. Notice the commit with subject "Add bin/physical.sh". Call that the checkpoint commit. There are no file movements before the checkpoint commit. The four commits following the checkpoint commit are generated by running the bin/physical.sh script added in the checkpoint commit.

When you want to merge, you should:

Then you can git merge -X ours $upstream where upstream points to the head of this branch. It will automatically resolve conflicts in your branch's favor. Then you can git diff $upstream and see your work ported.

This PR requires the same finishing steps as #4966:

codecov-commenter commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 71.3%. Comparing base (c706926) to head (714bee1).

:exclamation: Current head 714bee1 differs from pull request most recent head 7cf4611

Please upload reports for the commit 7cf4611 to get more accurate results.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/XRPLF/rippled/pull/4997/graphs/tree.svg?width=650&height=150&src=pr&token=i2RPGI5xGF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)](https://app.codecov.io/gh/XRPLF/rippled/pull/4997?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) ```diff @@ Coverage Diff @@ ## develop #4997 +/- ## ======================================= Coverage 71.3% 71.3% ======================================= Files 796 796 Lines 66987 66987 Branches 10886 10865 -21 ======================================= + Hits 47765 47791 +26 + Misses 19222 19196 -26 ``` | [Files](https://app.codecov.io/gh/XRPLF/rippled/pull/4997?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) | Coverage Δ | | |---|---|---| | [include/xrpl/basics/BasicConfig.h](https://app.codecov.io/gh/XRPLF/rippled/pull/4997?src=pr&el=tree&filepath=include%2Fxrpl%2Fbasics%2FBasicConfig.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-aW5jbHVkZS94cnBsL2Jhc2ljcy9CYXNpY0NvbmZpZy5o) | `87.7% <ø> (ø)` | | | [include/xrpl/basics/Buffer.h](https://app.codecov.io/gh/XRPLF/rippled/pull/4997?src=pr&el=tree&filepath=include%2Fxrpl%2Fbasics%2FBuffer.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-aW5jbHVkZS94cnBsL2Jhc2ljcy9CdWZmZXIuaA==) | `100.0% <ø> (ø)` | | | [include/xrpl/basics/ByteUtilities.h](https://app.codecov.io/gh/XRPLF/rippled/pull/4997?src=pr&el=tree&filepath=include%2Fxrpl%2Fbasics%2FByteUtilities.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-aW5jbHVkZS94cnBsL2Jhc2ljcy9CeXRlVXRpbGl0aWVzLmg=) | `100.0% <ø> (ø)` | | | [include/xrpl/basics/CompressionAlgorithms.h](https://app.codecov.io/gh/XRPLF/rippled/pull/4997?src=pr&el=tree&filepath=include%2Fxrpl%2Fbasics%2FCompressionAlgorithms.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-aW5jbHVkZS94cnBsL2Jhc2ljcy9Db21wcmVzc2lvbkFsZ29yaXRobXMuaA==) | `0.0% <ø> (ø)` | | | [include/xrpl/basics/CountedObject.h](https://app.codecov.io/gh/XRPLF/rippled/pull/4997?src=pr&el=tree&filepath=include%2Fxrpl%2Fbasics%2FCountedObject.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-aW5jbHVkZS94cnBsL2Jhc2ljcy9Db3VudGVkT2JqZWN0Lmg=) | `100.0% <ø> (ø)` | | | [include/xrpl/basics/DecayingSample.h](https://app.codecov.io/gh/XRPLF/rippled/pull/4997?src=pr&el=tree&filepath=include%2Fxrpl%2Fbasics%2FDecayingSample.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-aW5jbHVkZS94cnBsL2Jhc2ljcy9EZWNheWluZ1NhbXBsZS5o) | `87.9% <ø> (ø)` | | | [include/xrpl/basics/Expected.h](https://app.codecov.io/gh/XRPLF/rippled/pull/4997?src=pr&el=tree&filepath=include%2Fxrpl%2Fbasics%2FExpected.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-aW5jbHVkZS94cnBsL2Jhc2ljcy9FeHBlY3RlZC5o) | `100.0% <ø> (ø)` | | | [include/xrpl/basics/FeeUnits.h](https://app.codecov.io/gh/XRPLF/rippled/pull/4997?src=pr&el=tree&filepath=include%2Fxrpl%2Fbasics%2FFeeUnits.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-aW5jbHVkZS94cnBsL2Jhc2ljcy9GZWVVbml0cy5o) | `90.1% <ø> (ø)` | | | [include/xrpl/basics/IOUAmount.h](https://app.codecov.io/gh/XRPLF/rippled/pull/4997?src=pr&el=tree&filepath=include%2Fxrpl%2Fbasics%2FIOUAmount.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-aW5jbHVkZS94cnBsL2Jhc2ljcy9JT1VBbW91bnQuaA==) | `100.0% <ø> (ø)` | | | [include/xrpl/basics/LocalValue.h](https://app.codecov.io/gh/XRPLF/rippled/pull/4997?src=pr&el=tree&filepath=include%2Fxrpl%2Fbasics%2FLocalValue.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF#diff-aW5jbHVkZS94cnBsL2Jhc2ljcy9Mb2NhbFZhbHVlLmg=) | `100.0% <ø> (ø)` | | | ... and [158 more](https://app.codecov.io/gh/XRPLF/rippled/pull/4997?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) | | ... and [1256 files with indirect coverage changes](https://app.codecov.io/gh/XRPLF/rippled/pull/4997/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF) [![Impacted file tree graph](https://app.codecov.io/gh/XRPLF/rippled/pull/4997/graphs/tree.svg?width=650&height=150&src=pr&token=i2RPGI5xGF&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)](https://app.codecov.io/gh/XRPLF/rippled/pull/4997?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XRPLF)
Bronek commented 4 months ago

Note, this is moving files from impl and details to their parent directories, which I think is a problem because that expands the libxrpl API surface with the following files (which were designed to be implementation and not API)

json/impl/json_assert.h

protocol/impl/STVar.h  protocol/impl/b58_utils.h  protocol/impl/secp256k1.h  protocol/impl/token_errors.h

resource/impl/Entry.h  resource/impl/Import.h  resource/impl/Key.h  resource/impl/Kind.h  resource/impl/Logic.h  resource/impl/Tuning.h

server/impl/BaseHTTPPeer.h  server/impl/BaseWSPeer.h  server/impl/JSONRPCUtil.h  server/impl/PlainHTTPPeer.h  server/impl/SSLHTTPPeer.h  server/impl/ServerImpl.h
server/impl/BasePeer.h      server/impl/Door.h        server/impl/LowestLayer.h  server/impl/PlainWSPeer.h    server/impl/SSLWSPeer.h    server/impl/io_list.h
Bronek commented 3 months ago

Note, this is moving files from impl and details to their parent directories, which I think is a problem because that expands the libxrpl API surface with the following files (which were designed to be implementation and not API)

json/impl/json_assert.h

protocol/impl/STVar.h  protocol/impl/b58_utils.h  protocol/impl/secp256k1.h  protocol/impl/token_errors.h

resource/impl/Entry.h  resource/impl/Import.h  resource/impl/Key.h  resource/impl/Kind.h  resource/impl/Logic.h  resource/impl/Tuning.h

server/impl/BaseHTTPPeer.h  server/impl/BaseWSPeer.h  server/impl/JSONRPCUtil.h  server/impl/PlainHTTPPeer.h  server/impl/SSLHTTPPeer.h  server/impl/ServerImpl.h
server/impl/BasePeer.h      server/impl/Door.h        server/impl/LowestLayer.h  server/impl/PlainWSPeer.h    server/impl/SSLWSPeer.h    server/impl/io_list.h

@thejohnfreeman thanks for addressing this concern.

Bronek commented 3 months ago

I noticed that the most recent rebase has reverted the change to which I refer here #4997 (comment)

Thanks @thejohnfreeman , I see that's fixed now.

scottschurr commented 3 months ago

I'm trying to follow the directions for merging an in-process branch with the physical restructure. Everything was going well until I ran bin/physical.sh. There I had two surprises.

The output from bin/physical.sh was this:

move protocol buffers
move libxrpl headers
move libxrpl sources
check leftovers
leftover files:
beast//utility/src/beast_PropertyStream.cpp crypto//impl/secure_erase.cpp

The leftover files was a surprise, and the instructions did not say what to do if that happened, so I'm reporting it here.

Additionally, I checked the sort order of Builds/levelization/results/ordering.txt as instructed. To my surprise it contained no lines that started with either xrpl. or xrpld. All 229 lines in the file started with either ripple. or test.. So I suspect something went wrong.

Suggestions? Thanks.

thejohnfreeman commented 3 months ago

@scottschurr what branch did you try?

scottschurr commented 3 months ago

@thejohnfreeman, this is the branch I tried: https://github.com/scottschurr/rippled/tree/blocked-book

thejohnfreeman commented 3 months ago

I just merged that branch without a hitch: https://github.com/thejohnfreeman/rippled/tree/blocked-book

I'll try to replicate your experience on macOS soon.

scottschurr commented 3 months ago

Thanks. And if there are any experiments you would like me to try I'm happy to do so. Best of luck.

thejohnfreeman commented 3 months ago

The problem is what I expected, the environment. The requirements I found missing on macOS are:

Can you run the script in a Linux environment? Alternatively, if you can get these programs on your PATH (brew install gnu-sed findutils will install gsed and gfind; Homebrew doesn't have clang-format-10) and invoke the script with Bash (the default shell in macOS is now Zsh), then it should work.

scottschurr commented 3 months ago

I don't have a readily available Linux environment, but I can install gsed and gfind using homebrew and invoke the script with Bash. I'm in the middle of a different (unrelated) problem right now, but I should be able to get you results in a day or so. Thanks for the help.

scottschurr commented 3 months ago

I just checked my Mac for the versions of some of the utilities the conversion script uses:

$ awk --version
awk version 20200816
$ bash --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin21)
Copyright (C) 2007 Free Software Foundation, Inc.
$ what /usr/bin/find
/usr/bin/find
    PROGRAM:find  PROJECT:shell_cmds-240.100.15
    PROGRAM:find  PROJECT:shell_cmds-240.100.15
$ what /usr/bin/sed
/usr/bin/sed
    PROGRAM:sed  PROJECT:text_cmds-138.100.3
    PROGRAM:sed  PROJECT:text_cmds-138.100.3

It sounds like the version of bash is just not gonna work with your script. I'm less sure about the others. Thoughts?

thejohnfreeman commented 3 months ago

On my Mac, I installed Bash from Homebrew. It has version 5.2. Mac's versions of find and sed come from FreeBSD (to my knowledge) with version strings that are inscrutable to me. The awk may be compatible.

As a reminder, I'm willing to manually merge anyone's in-progress branch on demand. They just need to resolve the conflicts up to whatever commit precedes the merge of this PR.