From 638daa87fe2339d3542c184183f813d066203752 Mon Sep 17 00:00:00 2001 From: Sean Date: Sat, 5 Sep 2020 05:46:25 +0000 Subject: [PATCH 1/3] Avoid Printing Binary String to Logs (#1576) Converts the graffiti binary data to string before printing to logs. ## Issue Addressed #1566 ## Proposed Changes Rather than converting graffiti to a vector the binary data less the last character is passed to String::from_utf_lossy(). This then allows us to call the to_string() function directly to give us the string ## Additional Info Rust skills are fairly weak --- Cargo.lock | 1 + beacon_node/beacon_chain/Cargo.toml | 1 + beacon_node/beacon_chain/src/beacon_chain.rs | 8 ++++++-- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e6285fbb408..e7156ecd975 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -348,6 +348,7 @@ dependencies = [ "rand 0.7.3", "rand_core 0.5.1", "rayon", + "regex", "safe_arith", "serde", "serde_derive", diff --git a/beacon_node/beacon_chain/Cargo.toml b/beacon_node/beacon_chain/Cargo.toml index 04e22f4268f..05ae819c49c 100644 --- a/beacon_node/beacon_chain/Cargo.toml +++ b/beacon_node/beacon_chain/Cargo.toml @@ -58,3 +58,4 @@ environment = { path = "../../lighthouse/environment" } bus = "2.2.3" derivative = "2.1.1" itertools = "0.9.0" +regex = "1.3.9" diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index b6320ad5295..1f8e8f33b6e 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -31,6 +31,7 @@ use fork_choice::ForkChoice; use itertools::process_results; use operation_pool::{OperationPool, PersistedOperationPool}; use parking_lot::RwLock; +use regex::bytes::Regex; use slog::{crit, debug, error, info, trace, warn, Logger}; use slot_clock::SlotClock; use state_processing::{ @@ -1319,8 +1320,11 @@ impl BeaconChain { block: SignedBeaconBlock, ) -> Result, BlockError> { let slot = block.message.slot; - let graffiti_string = String::from_utf8(block.message.body.graffiti[..].to_vec()) - .unwrap_or_else(|_| format!("{:?}", &block.message.body.graffiti[..])); + #[allow(clippy::invalid_regex)] + let re = Regex::new("\\p{C}").expect("regex is valid"); + let graffiti_string = + String::from_utf8_lossy(&re.replace_all(&block.message.body.graffiti[..], &b""[..])) + .to_string(); match GossipVerifiedBlock::new(block, self) { Ok(verified) => { From 211109bbc05eebf337641b14007a48aa19a816e5 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Sun, 6 Sep 2020 04:46:25 +0000 Subject: [PATCH 2/3] Revert "add a github action for build multi-arch docker images (#1574)" (#1591) This reverts commit 26274633660a372429abee7be4b0ae1f7c13c0e1. ## Issue Addressed This is a temporary fix for #1589, by reverting #1574. The Docker image needs to be built with `--build-arg PORTABLE=true`, and we could probably integrate that into the multi-arch build, but in the interests of expediting a fix, this PR opts for a revert. --- .github/workflows/docker.yml | 42 ------------------------------------ 1 file changed, 42 deletions(-) delete mode 100644 .github/workflows/docker.yml diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml deleted file mode 100644 index d03b95e8d71..00000000000 --- a/.github/workflows/docker.yml +++ /dev/null @@ -1,42 +0,0 @@ -name: docker - -on: - push: - branches: - - master - -jobs: - build-docker-image: - runs-on: ubuntu-18.04 - steps: - - uses: actions/checkout@master - - - name: Dockerhub login - env: - DOCKER_PASSWORD: ${{ secrets.DOCKER_PASSWORD }} - DOCKER_USERNAME: ${{ secrets.DOCKER_USERNAME }} - run: | - echo "${DOCKER_PASSWORD}" | docker login --username ${DOCKER_USERNAME} --password-stdin - - - name: Build Docker Buildx - run: | - export DOCKER_BUILDKIT=1; - docker build --platform=local -o . git://github.com/docker/buildx; - mkdir -p ~/.docker/cli-plugins; - mv buildx ~/.docker/cli-plugins/docker-buildx; - - - name: Create Docker Builder - run: | - docker run --rm --privileged multiarch/qemu-user-static --reset -p yes; - docker context create node-amd64; - docker context create node-arm64; - docker buildx create --use --name lighthouse node-amd64; - docker buildx create --append --name lighthouse node-arm64; - - - name: Build dockerfile (with push) - run: | - docker buildx build \ - --platform=linux/amd64,linux/arm64 \ - --output "type=image,push=true" \ - --file ./Dockerfile . \ - --tag sigp/lighthouse:latest From 74fa87aa98bfa20cf18c1a99a3b33d21d460b705 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 7 Sep 2020 01:03:53 +0000 Subject: [PATCH 3/3] Add serde_utils module with quoted u64 support (#1588) ## Proposed Changes This is an extraction of the quoted int code from #1569, that I've come to rely on for #1544. It allows us to parse integers from serde strings in YAML, JSON, etc. The main differences from the code in Paul's original PR are: * Added a submodule that makes quoting mandatory (`require_quotes`). * Decoding is generic over the type `T` being decoded. You can use `#[serde(with = "serde_utils::quoted_u64::require_quotes")]` on `Epoch` and `Slot` fields (this is what I do in my slashing protection PR). I've turned on quoting for `Epoch` and `Slot` in this PR, but will leave the other `types` changes to you Paul. I opted to put everything in the `conseus/serde_utils` module so that BLS can use it without a circular dependency. In future when we want to publish `types` I think we could publish `serde_utils` as `lighthouse_serde_utils` or something. Open to other ideas on this front too. --- Cargo.lock | 10 ++ Cargo.toml | 1 + consensus/serde_utils/Cargo.toml | 12 ++ consensus/serde_utils/src/lib.rs | 2 + consensus/serde_utils/src/quoted_u64.rs | 115 ++++++++++++++++++++ consensus/serde_utils/src/quoted_u64_vec.rs | 91 ++++++++++++++++ consensus/types/Cargo.toml | 1 + consensus/types/src/slot_epoch.rs | 5 +- consensus/types/src/utils.rs | 2 +- 9 files changed, 236 insertions(+), 3 deletions(-) create mode 100644 consensus/serde_utils/Cargo.toml create mode 100644 consensus/serde_utils/src/lib.rs create mode 100644 consensus/serde_utils/src/quoted_u64.rs create mode 100644 consensus/serde_utils/src/quoted_u64_vec.rs diff --git a/Cargo.lock b/Cargo.lock index e7156ecd975..5d34cfad5e2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4668,6 +4668,15 @@ dependencies = [ "url 2.1.1", ] +[[package]] +name = "serde_utils" +version = "0.1.0" +dependencies = [ + "serde", + "serde_derive", + "serde_json", +] + [[package]] name = "serde_yaml" version = "0.8.13" @@ -5886,6 +5895,7 @@ dependencies = [ "serde", "serde_derive", "serde_json", + "serde_utils", "serde_yaml", "slog", "swap_or_not_shuffle", diff --git a/Cargo.toml b/Cargo.toml index 59bf507fa50..fec48a1349c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -44,6 +44,7 @@ members = [ "consensus/ssz_derive", "consensus/ssz_types", "consensus/serde_hex", + "consensus/serde_utils", "consensus/state_processing", "consensus/swap_or_not_shuffle", "consensus/tree_hash", diff --git a/consensus/serde_utils/Cargo.toml b/consensus/serde_utils/Cargo.toml new file mode 100644 index 00000000000..1fb35736baf --- /dev/null +++ b/consensus/serde_utils/Cargo.toml @@ -0,0 +1,12 @@ +[package] +name = "serde_utils" +version = "0.1.0" +authors = ["Paul Hauner "] +edition = "2018" + +[dependencies] +serde = { version = "1.0.110", features = ["derive"] } +serde_derive = "1.0.110" + +[dev-dependencies] +serde_json = "1.0.52" diff --git a/consensus/serde_utils/src/lib.rs b/consensus/serde_utils/src/lib.rs new file mode 100644 index 00000000000..df2b44b6243 --- /dev/null +++ b/consensus/serde_utils/src/lib.rs @@ -0,0 +1,2 @@ +pub mod quoted_u64; +pub mod quoted_u64_vec; diff --git a/consensus/serde_utils/src/quoted_u64.rs b/consensus/serde_utils/src/quoted_u64.rs new file mode 100644 index 00000000000..2e73a104f19 --- /dev/null +++ b/consensus/serde_utils/src/quoted_u64.rs @@ -0,0 +1,115 @@ +use serde::{Deserializer, Serializer}; +use serde_derive::{Deserialize, Serialize}; +use std::marker::PhantomData; + +/// Serde support for deserializing quoted integers. +/// +/// Configurable so that quotes are either required or optional. +pub struct QuotedIntVisitor { + require_quotes: bool, + _phantom: PhantomData, +} + +impl<'a, T> serde::de::Visitor<'a> for QuotedIntVisitor +where + T: From + Into + Copy, +{ + type Value = T; + + fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result { + if self.require_quotes { + write!(formatter, "a quoted integer") + } else { + write!(formatter, "a quoted or unquoted integer") + } + } + + fn visit_str(self, s: &str) -> Result + where + E: serde::de::Error, + { + s.parse::() + .map(T::from) + .map_err(serde::de::Error::custom) + } + + fn visit_u64(self, v: u64) -> Result + where + E: serde::de::Error, + { + if self.require_quotes { + Err(serde::de::Error::custom( + "received unquoted integer when quotes are required", + )) + } else { + Ok(T::from(v)) + } + } +} + +/// Wrapper type for requiring quotes on a `u64`-like type. +/// +/// Unlike using `serde(with = "quoted_u64::require_quotes")` this is composable, and can be nested +/// inside types like `Option`, `Result` and `Vec`. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Deserialize, Serialize)] +#[serde(transparent)] +pub struct Quoted +where + T: From + Into + Copy, +{ + #[serde(with = "require_quotes")] + pub value: T, +} + +/// Serialize with quotes. +pub fn serialize(value: &T, serializer: S) -> Result +where + S: Serializer, + T: From + Into + Copy, +{ + let v: u64 = (*value).into(); + serializer.serialize_str(&format!("{}", v)) +} + +/// Deserialize with or without quotes. +pub fn deserialize<'de, D, T>(deserializer: D) -> Result +where + D: Deserializer<'de>, + T: From + Into + Copy, +{ + deserializer.deserialize_any(QuotedIntVisitor { + require_quotes: false, + _phantom: PhantomData, + }) +} + +/// Requires quotes when deserializing. +/// +/// Usage: `#[serde(with = "quoted_u64::require_quotes")]`. +pub mod require_quotes { + pub use super::serialize; + use super::*; + + pub fn deserialize<'de, D, T>(deserializer: D) -> Result + where + D: Deserializer<'de>, + T: From + Into + Copy, + { + deserializer.deserialize_any(QuotedIntVisitor { + require_quotes: true, + _phantom: PhantomData, + }) + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn require_quotes() { + let x = serde_json::from_str::>("\"8\"").unwrap(); + assert_eq!(x.value, 8); + serde_json::from_str::>("8").unwrap_err(); + } +} diff --git a/consensus/serde_utils/src/quoted_u64_vec.rs b/consensus/serde_utils/src/quoted_u64_vec.rs new file mode 100644 index 00000000000..c5badee5012 --- /dev/null +++ b/consensus/serde_utils/src/quoted_u64_vec.rs @@ -0,0 +1,91 @@ +use serde::ser::SerializeSeq; +use serde::{Deserializer, Serializer}; +use serde_derive::{Deserialize, Serialize}; + +#[derive(Serialize, Deserialize)] +#[serde(transparent)] +pub struct QuotedIntWrapper { + #[serde(with = "crate::quoted_u64")] + int: u64, +} + +pub struct QuotedIntVecVisitor; +impl<'a> serde::de::Visitor<'a> for QuotedIntVecVisitor { + type Value = Vec; + + fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result { + write!(formatter, "a list of quoted or unquoted integers") + } + + fn visit_seq(self, mut seq: A) -> Result + where + A: serde::de::SeqAccess<'a>, + { + let mut vec = vec![]; + + while let Some(val) = seq.next_element()? { + let val: QuotedIntWrapper = val; + vec.push(val.int); + } + + Ok(vec) + } +} + +pub fn serialize(value: &[u64], serializer: S) -> Result +where + S: Serializer, +{ + let mut seq = serializer.serialize_seq(Some(value.len()))?; + for &int in value { + seq.serialize_element(&QuotedIntWrapper { int })?; + } + seq.end() +} + +pub fn deserialize<'de, D>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + deserializer.deserialize_any(QuotedIntVecVisitor) +} + +#[cfg(test)] +mod test { + use super::*; + + #[derive(Debug, Serialize, Deserialize)] + struct Obj { + #[serde(with = "crate::quoted_u64_vec")] + values: Vec, + } + + #[test] + fn quoted_list_success() { + let obj: Obj = serde_json::from_str(r#"{ "values": ["1", "2", "3", "4"] }"#).unwrap(); + assert_eq!(obj.values, vec![1, 2, 3, 4]); + } + + #[test] + fn unquoted_list_success() { + let obj: Obj = serde_json::from_str(r#"{ "values": [1, 2, 3, 4] }"#).unwrap(); + assert_eq!(obj.values, vec![1, 2, 3, 4]); + } + + #[test] + fn mixed_list_success() { + let obj: Obj = serde_json::from_str(r#"{ "values": ["1", 2, "3", "4"] }"#).unwrap(); + assert_eq!(obj.values, vec![1, 2, 3, 4]); + } + + #[test] + fn empty_list_success() { + let obj: Obj = serde_json::from_str(r#"{ "values": [] }"#).unwrap(); + assert!(obj.values.is_empty()); + } + + #[test] + fn whole_list_quoted_err() { + serde_json::from_str::(r#"{ "values": "[1, 2, 3, 4]" }"#).unwrap_err(); + } +} diff --git a/consensus/types/Cargo.toml b/consensus/types/Cargo.toml index d893ff3ad03..6334eaf4a83 100644 --- a/consensus/types/Cargo.toml +++ b/consensus/types/Cargo.toml @@ -25,6 +25,7 @@ rand = "0.7.3" safe_arith = { path = "../safe_arith" } serde = "1.0.110" serde_derive = "1.0.110" +serde_utils = { path = "../serde_utils" } slog = "2.5.2" eth2_ssz = "0.1.2" eth2_ssz_derive = "0.1.0" diff --git a/consensus/types/src/slot_epoch.rs b/consensus/types/src/slot_epoch.rs index 42b922cfbc2..cec17b09a5b 100644 --- a/consensus/types/src/slot_epoch.rs +++ b/consensus/types/src/slot_epoch.rs @@ -25,11 +25,12 @@ use std::ops::{Add, AddAssign, Div, DivAssign, Mul, MulAssign, Rem, Sub, SubAssi #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] #[derive(Eq, Clone, Copy, Default, Serialize, Deserialize)] #[serde(transparent)] -pub struct Slot(u64); +pub struct Slot(#[serde(with = "serde_utils::quoted_u64")] u64); #[cfg_attr(feature = "arbitrary-fuzz", derive(arbitrary::Arbitrary))] #[derive(Eq, Clone, Copy, Default, Serialize, Deserialize)] -pub struct Epoch(u64); +#[serde(transparent)] +pub struct Epoch(#[serde(with = "serde_utils::quoted_u64")] u64); impl_common!(Slot); impl_common!(Epoch); diff --git a/consensus/types/src/utils.rs b/consensus/types/src/utils.rs index 51af86692cb..a527fc18fd1 100644 --- a/consensus/types/src/utils.rs +++ b/consensus/types/src/utils.rs @@ -1,3 +1,3 @@ mod serde_utils; -pub use serde_utils::*; +pub use self::serde_utils::*;