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

TOOLS-1469 #27

Open
wants to merge 4 commits into
base: rosetta-zen
Choose a base branch
from

Conversation

otoumas
Copy link

@otoumas otoumas commented Oct 7, 2022

In this PR:

  • Included latest changes form rosetta-bitcoin see TOOLS-1469
  • Added log level endpoints
  • Removed hardcoded zend version numbers (now we only need to update it in make file)

Notes

  • We will use this same repository to implement the indexer for mainchain, first with a different branch and then with flags

@otoumas otoumas requested review from yurikoinaba and cronicc October 7, 2022 15:11
@otoumas otoumas requested a review from voyenavion October 17, 2022 15:22
ARG IS_RELEASE=false
# cronic <[email protected]> https://keys.openpgp.org/vks/v1/by-fingerprint/219F55740BBF7A1CE368BA45FB7053CE4991B669
# Luigi Varriale <[email protected]> https://keys.openpgp.org/vks/v1/by-fingerprint/FC3388A460ACFAB04E8328C07BB2A1D2CFDFCD2C
ARG MAINTAINER_KEYS="219F55740BBF7A1CE368BA45FB7053CE4991B669 FC3388A460ACFAB04E8328C07BB2A1D2CFDFCD2C"
# otoumas <[email protected]> https://keys.openpgp.org/vks/v1/by-fingerprint/2BBE2AA1A641F6147B58450BE3527B60DAACA1D8

Choose a reason for hiding this comment

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

too much info in the comments = security risk?

Copy link
Author

Choose a reason for hiding this comment

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

well these are our public keys, so I don't think it is a great risk

Copy link
Member

Choose a reason for hiding this comment

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

Note that MAINTAINER_KEYS here is used to check who is allowed to sign the release tag on the zend repository. Not rosetta-zen.

Including PaoloT in this list would make more sense.

// we multiply runtime.NumCPU by to determine
// how many goroutines we should
// spwan to handle block data sequencing.
overclockMultiplier = 16

Choose a reason for hiding this comment

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

I wonder if there would be a way to make this value a function of the underlying machine's capacity instead of a fixed hard-coded value? seems like a potential risk for running out of threads or memory, but I could be wrong. Is it always the case that 16 routines to one cpu is a good fit?

Copy link
Author

Choose a reason for hiding this comment

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

Agree, but we can come back here and improve this afterwards. At the moment just aligning with rosetta-bitcoin

opts.TableLoadingMode = options.MemoryMap
opts.ValueLogLoadingMode = options.MemoryMap
opts.TableLoadingMode = options.FileIO
opts.ValueLogLoadingMode = options.FileIO

Choose a reason for hiding this comment

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

Has something changed here to warrant a change of the comment? "FileIO" seems different and more also more vague than "MemoryMap"

Copy link
Author

Choose a reason for hiding this comment

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

This is a very good question @voyenavion , this is something I wanted @cronicc 's opinion on

Copy link
Member

Choose a reason for hiding this comment

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

I don't know about the reasoning for switching to FileIO here, it should be slower than MemoryMap, but have less memory requirements. Maybe there was a reason for the change provided in the upstream commit message?

ARG IS_RELEASE=false
# cronic <[email protected]> https://keys.openpgp.org/vks/v1/by-fingerprint/219F55740BBF7A1CE368BA45FB7053CE4991B669
# Luigi Varriale <[email protected]> https://keys.openpgp.org/vks/v1/by-fingerprint/FC3388A460ACFAB04E8328C07BB2A1D2CFDFCD2C
ARG MAINTAINER_KEYS="219F55740BBF7A1CE368BA45FB7053CE4991B669 FC3388A460ACFAB04E8328C07BB2A1D2CFDFCD2C"
# otoumas <[email protected]> https://keys.openpgp.org/vks/v1/by-fingerprint/2BBE2AA1A641F6147B58450BE3527B60DAACA1D8
Copy link
Member

Choose a reason for hiding this comment

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

Note that MAINTAINER_KEYS here is used to check who is allowed to sign the release tag on the zend repository. Not rosetta-zen.

Including PaoloT in this list would make more sense.

@@ -53,18 +53,18 @@ RUN set -euxo pipefail \
&& ( gpgconf --kill dirmngr || true ) \
&& ( gpgconf --kill gpg-agent || true ); \
fi \
&& export MAKEFLAGS="-j $(($(nproc)+1))" && ./zcutil/build.sh $MAKEFLAGS
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer making this a build time check, as legacy-cpu has a performance penalty. Any modern platform (cloud servers where Coinbase and we run rosetta-zen) supports the needed CPU flag. The only use case I see for building legacy-cpu images is on developer machines.

For detecting CPU support while running docker build an approach similar to this https://github.com/HorizenOfficial/zen-node-docker/blob/84c213dec9636d616721c971b74214ab75a66916/entrypoint.sh#L6 should work. Or https://manpages.ubuntu.com/manpages/focal/man1/lscpu.1.html can be used.

docker run --rm -v "${PWD}/zen-data:/data" ubuntu:18.04 bash -c 'chown -R nobody:nogroup /data';
docker run -d --name=rosetta-zen-testnet-online --ulimit "nofile=${NOFILE}:${NOFILE}" -v "${PWD}/zen-data:/data" -e "MODE=ONLINE" -e "NETWORK=TESTNET" -e "PORT=8080" -p 8080:8080 -p 19033:19033 rosetta-zen:latest;
docker run --rm -v "${PWD}/zen-data:/data" ubuntu:20.04 bash -c 'chown -R nobody:nogroup /data';
docker run -d --name=rosetta-zen-testnet-online --ulimit "nofile=${NOFILE}:${NOFILE}" -v "${PWD}/zen-data:/data" -e "MODE=ONLINE" -e "NETWORK=TESTNET" -e "PORT=8080" -p 8080:8080 -p 19033:19033 -p 18231:18231 rosetta-zen:latest;
Copy link
Member

Choose a reason for hiding this comment

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

RPC ports like -p 18231:18231 should never be exposed to the internet, which -p does if the instance rosetta is running on has a publicly routable IP. The RPC interface is unencrypted, and even though there is authentication, there is no rate limiting against brute force attacks. --expose 18231 can be used instead which exposes the port to other containers on the same docker network, or -p 127.0.0.1:18231 to only expose the port on localhost.

# start rosetta-zen
docker run -d --rm --ulimit "nofile=100000:100000" -v "$(pwd)/zen-data:/data" -e "MODE=ONLINE" -e "NETWORK=MAINNET" -e "PORT=8080" -p 8080:8080 -p 9033:9033 rosetta-zen:latest
# start rosetta-zen. Zend version needs to be specified (ex. v3.2.0)
docker run -d --rm --ulimit "nofile=100000:100000" -v "$(pwd)/zen-data:/data" -e "MODE=ONLINE" -e "NETWORK=MAINNET" -e "NODE_VERSION=${ZEND_VERSION}" -e "PORT=8080" -p 8080:8080 -p 9033:9033 rosetta-zen:latest
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to specify NODE_VERSION? I can't see it being used anywhere. Zend version could also be detected on the fly in entypoint.sh by running zend -version | head -n1 | cut -d " " -f 4, or storing the var from the Makefile via ENV NODE_VERSION=$ARG in Dockerfile.

opts.TableLoadingMode = options.MemoryMap
opts.ValueLogLoadingMode = options.MemoryMap
opts.TableLoadingMode = options.FileIO
opts.ValueLogLoadingMode = options.FileIO
Copy link
Member

Choose a reason for hiding this comment

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

I don't know about the reasoning for switching to FileIO here, it should be slower than MemoryMap, but have less memory requirements. Maybe there was a reason for the change provided in the upstream commit message?

@@ -154,6 +184,13 @@ func defaultBadgerOptions(
// filters will be immediately discarded from the cache).
opts.LoadBloomsOnOpen = false

// BlockSize sets the size of any block in SSTable. SSTable is divided into multiple blocks
// internally. We set each block to 1MB.
opts.BlockSize = 1 << blockLeftShift // 1MB
Copy link
Member

Choose a reason for hiding this comment

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

If this has any relationship to the size of blocks in our mainchain, 1MB would not be enough as we support larger blocks.

@@ -161,3 +141,61 @@ func TestAccountBalance_Online_Historical(t *testing.T) {

mockIndexer.AssertExpectations(t)
}

func TestAccountCoins_Online(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Accounts are deprecated in zend, all accounts are hard coded to empty string "" in RPC commands.

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.

5 participants