-
Notifications
You must be signed in to change notification settings - Fork 66
Problem: (fix #1177) chain-abci doesn't work with Tendermint v0.33.* #1441
Conversation
bors try |
tryBuild failed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
integration tests fail:
ERROR: Failed to create node: Error reading GenesisDoc at /drone/src/integration-tests/data/node0/tendermint/config/genesis.json: EvidenceParams.MaxAge must be greater than 0. Got 0
209 ERROR: Failed to create node: Error reading GenesisDoc at /drone/src/integration-tests/data/node0/tendermint/config/genesis.json: EvidenceParams.MaxAge must be greater than 0. Got 0
210 ERROR: Failed to create node: Error reading GenesisDoc at /drone/src/integration-tests/data/node0/tendermint/config/genesis.json: EvidenceParams.MaxAge must be greater than 0. Got 0
211 ERROR: Failed to create node: Error reading GenesisDoc at /drone/src/integration-tests/data/node0/tendermint/config/genesis.json: EvidenceParams.MaxAge must be greater than 0. Got 0
212 ERROR: Failed to create node: Error reading GenesisDoc at /drone/src/integration-tests/data/node0/tendermint/config/genesis.json: EvidenceParams.MaxAge must be greater than 0. Got 0
213 ERROR: Failed to create node: Error reading GenesisDoc at /drone/src/integration-tests/data/node0/tendermint/config/genesis.json: EvidenceParams.MaxAge must be greater than 0. Got 0
214 ERROR: Failed to create node: Error reading GenesisDoc at /drone/src/integration-tests/data/node0/tendermint/config/genesis.json: EvidenceParams.MaxAge must be greater than 0. Got 0
215 ERROR: Failed to create node: Error reading GenesisDoc at /drone/src/integration-tests/data/node0/tendermint/config/genesis.json: EvidenceParams.MaxAge must be greater than 0. Got 0
216 ERROR: Failed to create node: Error reading GenesisDoc at /drone/src/integration-tests/data/node0/tendermint/config/genesis.json: EvidenceParams.MaxAge must be greater than 0. Got 0
217 ERROR: Failed to create node: Error reading GenesisDoc at /drone/src/integration-tests/data/node0/tendermint/config/genesis.json: EvidenceParams.MaxAge must be greater than 0. Got 0
218 ERROR: Failed to create node: Error reading GenesisDoc at /drone/src/integration-tests/data/node0/tendermint/config/genesis.json: EvidenceParams.MaxAge must be greater than 0. Got 0
219 ERROR: Failed to create node: Error reading GenesisDoc at /drone/src/integration-tests/data/node0/tendermint/config/genesis.json: EvidenceParams.MaxAge must be greater than 0. Got 0
220
chain-core/src/init/config.rs
Outdated
@@ -80,7 +80,7 @@ impl fmt::Display for DistributionError { | |||
|
|||
/// Initial configuration ("app_state" in genesis.json of Tendermint config) | |||
/// TODO: reward/treasury config, extra validator config... | |||
#[derive(Debug, PartialEq, Eq, Clone)] | |||
#[derive(Debug, PartialEq, Eq, Default, Clone)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are defaults added to chain-core?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there seems to be an error when deserializing something if not add Default, I remember, I'll check again.
client-common/src/tendermint/lite.rs
Outdated
@@ -8,7 +8,7 @@ use crate::Result as CommonResult; | |||
|
|||
/// | |||
#[derive(Clone, Debug, Deserialize, Serialize)] | |||
pub struct TrustedState(pub(crate) Option<lite::TrustedState<SignedHeader, Header>>); | |||
pub struct TrustedState(pub Option<lite::TrustedState<SignedHeader, Header>>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not pub(crate)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that just used to get the Option<...>
to debug, I'll rollback.
@@ -147,7 +147,7 @@ impl AsyncRpcClient { | |||
/// TODO: Usage of `Vec` can be removed once we execute it in a purely async context | |||
pub async fn call_batch<T>(&self, batch_params: &[(&str, Vec<Value>)]) -> Result<Vec<T>> | |||
where | |||
for<'de> T: Deserialize<'de>, | |||
for<'de> T: Deserialize<'de> + std::fmt::Debug, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that debug needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just for debugging, I'll remove.
.map(|height| { | ||
( | ||
"validators", | ||
vec![json!(height.to_string()), json!("0"), json!("100")], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's that 0 and 100? paging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the rpc call changed, if not add 0, 100, there will be an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
page
and per_page
? I think we shouldn't have an assumptions of 100 validators?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default is 30, the max is 100, I don't know what value should be used here, I used the max value.
a8ba232
to
7db383d
Compare
we need to use tendermint v0.33 binary, the genesis also different with the old version. |
.map(|height| { | ||
( | ||
"validators", | ||
vec![json!(height.to_string()), json!("0"), json!("100")], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
page
and per_page
? I think we shouldn't have an assumptions of 100 validators?
}; | ||
|
||
let response_str = r#"{"height": "37", "txs_results": [{"code": 0, "data": null, "log": "", "info": "", "gasWanted": "0", "gasUsed": "0", "events": [{"type": "valid_txs", "attributes": [{"key": "ZmVl", "value": "MC4wMDAwMDMwNw=="}, {"key": "YWNjb3VudA==", "value": "MHgzMzUwMmVkMzlkMGM0ZTIwNDRmYjM3ZmRjZDUxNjE0OTNmNTkwMGMz"}, {"key": "dHhpZA==", "value": "ZjFmNzNkNmFjZWMyMTExOGRkMWUzNmY2ODRhYWUyMmM2Y2IxN2ZjNTFhZGEzNGEzNDIzMDlkNTMxY2I5YmU4ZA=="}]}], "codespace": ""}], "begin_block_events": null, "end_block_events": [{"type": "block_filter", "attributes": [{"key": "ZXRoYmxvb20=", "value": "AAAAAAAAAAAAAAAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=="}]}], "validator_updates": null, "consensus_param_updates": null}"#; | ||
// let response_str = r#" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this comment still needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I'll remove it.
assert!(result.is_ok()); | ||
assert_eq!(true, result.unwrap()); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to remove the following test cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, most of the test cases in this file is testing the invalid value of Attribute
in the BlockResults
and BlockResults
defined in this file.
Now, we use BlockResultsResponse
directly which defined in tendermint-rs
, and we query tendermint to get the BlockResultsResponse
, I don't think tendermint will give us one which value is invalid.
let s1 = serde_json::to_string_pretty(&state).unwrap(); | ||
let state2: std::result::Result<SyncState, _> = serde_json::from_str(&s1); | ||
let bytes = state.encode(); | ||
let _state1 = SyncState::decode(&mut bytes.as_slice()).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the assertion? And perhaps give some more meaningful variable names for readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added one.
client-core/src/wallet/syncer.rs
Outdated
@@ -327,7 +328,8 @@ impl<'a, S: SecureStorage, C: Client, D: TxDecryptor, F: FnMut(ProgressReport) - | |||
|
|||
let range = chunk.collect::<Vec<u64>>(); | |||
|
|||
if self.env.enable_fast_forward { | |||
//if self.env.enable_fast_forward { | |||
if true { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this for debug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I'll remove it
client-rpc/src/program.rs
Outdated
@@ -73,7 +73,7 @@ pub struct Options { | |||
|
|||
#[allow(dead_code)] | |||
pub fn run_cli() { | |||
env_logger::init(); | |||
// env_logger::init(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to disable env logger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a mistake. I'll recover it.
Can you modify the tendermint version number in the |
@@ -348,7 +348,8 @@ async def gen_genesis(cfg): | |||
"time_iota_ms": "1000" | |||
}, | |||
"evidence": { | |||
"max_age": "100000" | |||
"max_age_num_blocks": "100000", | |||
"max_age_duration": "172800000000000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the unit of max_age_duration
, I think we have an assumption that unbonding_period > max_age_duration
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use the default value of the new tendermint's config. we can find it in change log:
https://github.com/tendermint/tendermint/blob/master/CHANGELOG.md
test-common/src/block_generator.rs
Outdated
.expect("verify failed"); | ||
*/ | ||
|
||
//FIXME: use new interface in tendermint v0.33 instead of verify_single |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lite client API already migrated to tendermint-rs master, what's the difference in v0.33 on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v0.33 removed the interface and add some other similar interfaces, but the arguments are different, the logic also has some difference, I'm not clear about the logic and I think this function is very important, so I add FIXME here, maybe we can fix it in other PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v0.33 removed the interface and add some other similar interfaces, but the arguments are different, the logic also has some difference, I'm not clear about the logic and I think this function is very important, so I add FIXME here, maybe we can fix it in other PR.
I think this API still exists with the same signature?
https://github.com/crypto-com/tendermint-rs/blob/v0.33/tendermint/src/lite/verifier.rs#L228
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I'll check again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to fix gen_block()
to make this test pass.
Codecov Report
@@ Coverage Diff @@
## master #1441 +/- ##
==========================================
+ Coverage 63.43% 63.56% +0.13%
==========================================
Files 159 159
Lines 21188 21028 -160
==========================================
- Hits 13441 13367 -74
+ Misses 7747 7661 -86
|
bors try |
tryBuild failed: |
@@ -24,7 +24,7 @@ pub use tendermint::{ | |||
}; | |||
|
|||
/// crypto-com instantiated genesis type | |||
pub type Genesis = GenericGenesis<InitConfig>; | |||
pub type Genesis = GenericGenesis<Option<InitConfig>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this Option
needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before in tendermint-rs, it had a field Option<AppState>
-- now the type changed to AppState
, so it'd require Default implementation AppState
/ InitConfig
.
I think Option may be clearer, as it'll be immediate that the app_state was missing or incorrect in json rather than some arbitrary default value?
|
||
/// key space of wallet sync state | ||
const KEYSPACE: &str = "core_wallet_sync"; | ||
|
||
/// Sync state for wallet | ||
#[derive(Debug, Encode, Decode)] | ||
#[derive(Debug, Serialize, Deserialize, Encode, Decode)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are these two instances used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's for debugging, I'll remove.
@@ -13,7 +13,7 @@ RUN set -e; \ | |||
apt-get install -y libzmq3-dev libssl1.1 libprotobuf10 libsgx-launch libsgx-urts libsgx-epid libsgx-quote-ex; \ | |||
rm -rf /var/lib/apt/lists/* | |||
|
|||
COPY --from=tendermint/tendermint:v0.32.9 /usr/bin/tendermint /usr/bin/tendermint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 44 also needs to be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I'll do that.
test-common/src/block_generator.rs
Outdated
app_hash: Hash::new(hash::Algorithm::Sha256, &app_hash) | ||
.unwrap() | ||
.as_bytes() | ||
.to_vec(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app_hash: Hash::new(hash::Algorithm::Sha256, &app_hash) | |
.unwrap() | |
.as_bytes() | |
.to_vec(), | |
app_hash: app_hash.to_vec(), |
test-common/src/block_generator.rs
Outdated
app_hash: Hash::new(hash::Algorithm::Sha256, &last_state.last_apphash) | ||
.unwrap() | ||
.as_bytes() | ||
.to_vec(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app_hash: Hash::new(hash::Algorithm::Sha256, &last_state.last_apphash) | |
.unwrap() | |
.as_bytes() | |
.to_vec(), | |
app_hash: last_state.last_apphash.to_vec(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I think the lite client API is not changed, so the lite client code might not need to change
- Several maybe not used instance
- Other minor changes.
bors retry |
tryBuild failed: |
I guess the images on docker hub will need to be updated, I'll try |
tryBuild failed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
multinode integration test fails:
Traceback (most recent call last):
110 File "./multinode/join_test.py", line 45, in <module>
111 wait_for_validators(rpc, 3)
112 File "/drone/src/integration-tests/multinode/common.py", line 34, in wait_for_validators
113 n = len(rpc.chain.validators()['validators'])
114 File "/drone/src/integration-tests/bot/chainrpc.py", line 283, in validators
115 return self.call_chain('validators', str(height) if height is not None else None)
116 File "/drone/src/integration-tests/bot/chainrpc.py", line 62, in call_chain
117 rsp = request(self.chain_rpc_url, method, *args, **kwargs)
118 File "/drone/src/drone/venv/lib/python3.8/site-packages/jsonrpcclient/__init__.py", line 8, in request
119 return HTTPClient(endpoint).request(*args, **kwargs)
120 File "/drone/src/drone/venv/lib/python3.8/site-packages/apply_defaults/decorators.py", line 13, in wrapper
121 return function(self, *args, **kwargs)
122 File "/drone/src/drone/venv/lib/python3.8/site-packages/jsonrpcclient/client.py", line 229, in request
123 return self.send(
124 File "/drone/src/drone/venv/lib/python3.8/site-packages/apply_defaults/decorators.py", line 13, in wrapper
125 return function(self, *args, **kwargs)
126 File "/drone/src/drone/venv/lib/python3.8/site-packages/jsonrpcclient/client.py", line 177, in send
127 raise ReceivedErrorResponseError(response.data)
128 jsonrpcclient.exceptions.ReceivedErrorResponseError: Invalid params
@@ -8,23 +8,23 @@ platform: | |||
|
|||
steps: | |||
- name: build | |||
image: cryptocom/chain-test:v1.1.1 | |||
image: cryptocom/chain-test:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We better to increment the version number after pushed the image, otherwise, the image might not get updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not matter, Tomas can cover for that.
bors retry |
tryBuild failed: |
It seems that the |
bors retry |
tryBuild failed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
integration tests still fail + there's a conflict with the latest master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
63b31ef
to
e5005ae
Compare
bors try |
tryBuild failed: |
|
bors try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@linfeng-crypto can you squash the commits?
tryBuild succeeded: |
…t v0.33.* * update tendermint version in Dockerfile * fix test case, remove useless code * update docker image version and signature in .drone.yaml * fix check_lite_client test case * fix parameters for validators rpc in chainprc.py * remove useless function in block_generator * update tendermint-rs and fix a data error in multi_node test
5fe89a2
to
696d8cf
Compare
bors r+ |
Build succeeded: |
solution: