Fix double refund #2690
Open
Fix double refund #2690
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…#2577) ## Describe your changes and provide context fix: race condition and graceful shutdown in memiavl - reorder Close() to wait for background goroutine before closing resources - add context support to LoadMultiTree and Catchup for cancellation - fix resource leaks: close returned mtree, stop snapshotWriterPool ## Testing performed to validate your change
## Describe your changes and provide context Refactor the ante handler logics for DeliverTx into distinct flows. No check is supposed to be added or removed in this PR, but the ordering of checks have changed, which would result in different error codes for transactions that are invalid in multiple ways. As a result, this change is consensus breaking. The next step is to simplify the actual execution path of DeliverTx ## Testing performed to validate your change refactor; existing tests
New implementation uses the same representation as the old one, so it is backward compatible. * Made repair an implementation detail of the WAL. * Made WAL guarantee crash-tolerant atomicity (under assumption that the file system guarantees crash-tolerant atomic appends; if not, torn append writes are detected best effort by using crc32) * Added file locking to prevent concurrent access to WAL * Removed the background fsync task - background syncing is executed by the file system anyway (afaik), and we fsync multiple times a second (before signing) anyway. * Removed autofile and its weird semantics of autoclosing files which are not accessed after a delay - afaiu the idea was that one could hold ephemeral descriptors to all WAL files, not just the head, at all times. I've replaced it with explicit fd management. * simplified consensus WAL reads to only support reading last height messages - it is the only case used anyway.
…#2586) We removed sample app and some simulation logic from cosmos as part of: * #2507 ...but forgot to remove the corresponding .proto files. As a result, running `go generate ./...` in root results in unexpected go file generation. A separate work will investigate as to why UCI did not pick this issue up at the time.
Use `AppName` instead of `ServerName` to correctly populate the fields in built binary.
## Describe your changes and provide context Cosmos simulation logics were removed in an earlier commit https://github.com/sei-protocol/sei-chain/pull/2507/files#diff-0f1d2976054440336a576d47a44a37b80cdf6701dd9113012bce0e3c425819b7 However these logics are still needed by Cosmos-based wallets, so we cannot remove them just yet. ## Testing performed to validate your change add back old logics
Only ed25519 will be supported (it is the only enabled validator key scheme on atlantic-2,arctic-1 and pacific-1). This will simplify migration to autobahn. From now on crypto.PubKey is an alias of ed25519.PubKey. The pr is almost entirely generated with codex.
## Describe your changes and provide context Setting gas meter should be the very first thing when handling a transaction. If that's not the case and the handler exits for whatever reason, the upstream logic will use the common gas meter set at the beginning of `FinalizeBlock` as the transaction's gas meter, which can cause unexpected results. ## Testing performed to validate your change local cluster
## Describe your changes and provide context - set prices currently may not have been called by the time reads are attempted - this can cause a panic due to a race condition ## Testing performed to validate your change - new unit test
Test was assuming that state was in CommitPhase after marking block as final, while it could be in the previous phase as well. I've just removed the unreliable assertion - the externally visible state was asserted correctly anyway.
Rebuild the dynamic and static libraries of libwasmvm from rust code and replace the existing. Specifically, rebuilt from the current head of main 24912e1.
## Describe your changes and provide context Simply remove the assertion for async writes that is racing. When doing async writes, we should not assume that the intermediate log index is always falling behind or keeping up, it depends on the speed, we should just look at the correctness at the end for eventual consistency. ## Testing performed to validate your change
#2598) Simplify the root Dockerfile to use dynamic libraries checked into code. Improve caching layer and security. Remove redundant dependency installations.
…essure (#2591) ## Describe your changes and provide context - add early rejection for pruned blocks to avoid wasting resources - align DB semaphore size with worker pool size - add backpressure mechanism to reject requests when system is overloaded - add comprehensive worker pool metrics (Prometheus + optional stdout debug) - add DB semaphore tracking for I/O monitoring - add EVM_DEBUG_METRICS env var to enable debug output to stdout(nothing print out by default) ## Testing performed to validate your change - significant perf improvement on eth_getlog rpc - the latency average dropped by like 90%+ as we introduced early rejection to avoid resource waste - the nodes are much more responsive, very few situations where they’re lagging --------- Co-authored-by: Yiming Zang <50607998+yzang2019@users.noreply.github.com> Co-authored-by: yzang2019 <zymfrank@gmail.com>
## Describe your changes and provide context Drops the unused `totalCheckTxCount` counter from the mempool. This metric tracks the total number of transactions that the mempool asks the app to check. The figure isn't ever read, and I can't really think of a reason why we might ever want to use it, so I propose dropping it. ## Testing performed to validate your change `make test` succeeds.
To reduce noise considering code removal efforts.
## Describe your changes and provide context
This test can flakily fail with the following error:
```
--- FAIL: TestRouter_dialPeer_Reject (0.14s)
--- FAIL: TestRouter_dialPeer_Reject/incompatible_channels (0.02s)
router_test.go:548: want EOF, got mconn.Run: recvRoutine: protoReader.ReadMsg(): read tcp 127.0.0.1:41769->127.0.0.1:44942: read: connection reset by peer
```
The issue is that the test only wants an EOF error to signal a
connection being closed, but you can probably get a bunch of different
errors. This PR weakens the fail condition to `utils.IgnoreCancel(err)
== nil`, matching the pattern we already have in TestRouter_AcceptPeers
(see
[here](https://github.com/sei-protocol/sei-chain/blob/4bc58d5aee5cdf4ed02ed9228138b7960e82bc69/sei-tendermint/internal/p2p/router_test.go#L383)).
This PR also makes the same change to `TestRouter_EvictPeers`, which
might flake in a similar way.
## Testing performed to validate your change
`make test` passes
Add functionality that enables automatic backporting functionality by labeling the PRs. Once a labeled PR merges, the CI will attempt to automatically create the backport PR. The label must be in the following format: `backport <target-release-branch>`, where `<target-release-branch>` must pre-exist and match the name of the... yes you guessed it _the_ target release branch.
The IBC docs were originally forked from the upstream repo and are unchanged in the last 3 years. Remove them to reduce noise.
## Describe your changes and provide context Pretty much as in the title -- the CommitHash now covers all fields. ## Testing performed to validate your change Added a few new tests, `make test` passes.
## Describe your changes and provide context As in [the issue](https://linear.app/seilabs/team/CON/cycle/active), this PR adds a simple benchmark test, built on top of the existing OCC benchmark. It runs 50 conflicting, and 50 non-conflicting EVM transactions, shuffled together, a series of times, and reports on the time and gas performance: ``` 19 559324204 ns/op 4200000 gas/op 7509092 gas/sec 200.0 txns/op 344049411 B/op 5420234 allocs/op ``` The numbers are obviously highly dependent on: - machine specs - the number of conflicting vs. non-conflicting txs used ## Testing performed to validate your change N/A
## Describe your changes and provide context This PR: - changes mempool config to have `CheckTxErrorBlacklistEnabled` true by default - weakens blacklist protection to apply to only oversized transactions. Future PRs will expand blacklist protection to cover other forms of "clearly bad" transactions, such as: - undecodeable transactions - incorrect signatures - ludicrous params (more gas than a block has) - etc. ## Testing performed to validate your change `make test` passes
…on (#2604) ## Describe your changes and provide context MemStats and its returned HeapAlloc are inherently unreliable, so the test assertion here is always going to be flaky. I think `AllocsPerRun` is the right testing tool to use for what we want here, it didn't flake upon being run 2000 times locally.
Remove another unnecessary nesting of go.mod for sei-ibc-go Fix various bugs, lint and security issues.
For signing protobuf messages we need a well-defined canonical encoding. This PR introduces such encoding, by introducing the following rules: * only proto3 is supported * all fields have to have explicit presence (optional/repeated/oneof) * float/group/map fields are not supported * fields are encoded in tag ordering * unknown fields are ignored when encoding * repeated fields are packed if possible, except for singletons - this way changing field cardinality optional -> repeated is backward compatible (as usual) The encoding is using ProtoReflect, which is also used by the standard golang encoding (although with some trickery), therefore it should be efficient enough. We can switch to generated code later if needed. The rules above do not apply to ALL protobuf messages, just the ones marked with "hashable" option. Also this functionality is incompatible with sei-chain gogo generators (which ignore explicit presence), therefore a new package was introduced, which will use a standard golang generator. This package is currently unused, it will be used for new (autobahn) message types only.
## Describe your changes and provide context
Adds new test that verifies the disabling of instantiation & upload-code
of wasm is honored. It also verifies pointers are still invokable and
that querying still works.
```
Disable WASM Test
Pre-flight Check
✔ should have WASM enabled (Everybody) at the start (57ms)
✔ should be able to store and instantiate wasm when enabled (4698ms)
Disable WASM Scenario
✔ should disable WASM via governance (11461ms)
✔ should have wasm params set to Nobody (56ms)
✔ should fail to store new wasm code when disabled (2491ms)
✔ should fail to instantiate new contracts when disabled (1419ms)
✔ should still allow querying existing CW20 pointer when wasm is disabled
✔ should still allow executing on existing CW20 pointer when wasm is disabled (2370ms)
Re-enable WASM Scenario
✔ should re-enable WASM via governance (12249ms)
✔ should have wasm params set back to Everybody (56ms)
✔ should be able to store wasm after re-enabling (1830ms)
✔ should be able to instantiate contracts after re-enabling (4151ms)
✔ should still allow existing pointer to work after re-enabling
Two-step Proposal Process
✔ should be able to create and pass disable proposal in separate steps (24115ms)
✔ should be able to create and pass enable proposal in separate steps (24031ms)
Final State Verification
✔ should end with WASM enabled (Everybody) (56ms)
## Describe your changes and provide context The current flow is: - call `assertNotSealed` - `assertNotSealed` gets lock, checks sealed - `assertNotSealed` releases lock, returns - caller unsafely handles config without holding lock The new flow is: - get lock - call assertNotSealed, which assumes caller holds lock - assertNotSealed checks sealed, returns - caller safely handles config, releases lock upon return ## Testing performed to validate your change
## Describe your changes and provide context ## Testing performed to validate your change
- Introduce backend-agnostic DB/Batch/Iterator interfaces and ErrNotFound. - Add pebbledb adapter implementing the db_engine interfaces. - Remove legacy pebbledb/kv implementation. ## Describe your changes and provide context ## Testing performed to validate your change
The `seictl` utility is used extensively in the new infrastructure. Not having it would mean custom baking images which ultimately results in drift from test what we actually ship. Add it to the image as an "all in one" image. Its size is small enough to not matter for performance and portability.
## Describe your changes and provide context TSIA ## Testing performed to validate your change integration test
Add a CI workflow that publishes container images to an already set up immutable private ECR repo with security scanning enabled. The build is configured to: * Tag images by commit sha, ref -- since tags are explicitly configured to be immutable in ECR * Run on head of `release/*` and `main` branches * Dispatchable manually for ease of debugging Fixes PLT-59
## Describe your changes and provide context ## Testing performed to validate your change --------- Co-authored-by: Tony Chen <codchen03@gmail.com>
…#2680) ## Describe your changes and provide context Problem: Currently when a node app hashed, and the node was running in that state for a long enough time, snapshot creation will kick in on latest block height. And if the node was configured to only store 1 snapshot, then the old snapshot height will be truncated. That will lead to rollback failure because there's no way to rollback to a height that is older than the only snapshot exist. Mitigation: Previously we made a change to be able to create snapshot at any given height, as long as the height difference is > interval. This PR revert that changes so that snapshot creation will only happen if the block height % snapshot interval == 0. This should greatly reduce the chance of rollback failure leading to snapshot being created on latest height. ## Testing performed to validate your change
## Describe your changes and provide context As a result, validators using this mempool logic will produce blocks with a batch of EVM txs, followed by any non-EVM txs. This will allow us to use future executors that can only run EVM txs to run a bigger batch of EVM txs, falling back to existing executors for all non-EVM and interop behaviour. Note that: - this PR is NOT consensus-breaking. this is only a mempool operation, not a change to the rules of the protocol. - this PR does NOT change which txs the mempool reaps. the exact same txs are selected as before, just ordered to be a contiguous slab of EVM txs, followed by a slab of non-EVM ones. ## Testing performed to validate your change Added a new test, all tests pass
## Describe your changes and provide context - Removes unused hash range computation ## Testing performed to validate your change - Removed all files and verified no issues on running node - Verified backwards compatibility
RPC calls for the EVM CLI were attempting to send methods via GET instead of POST, resulting in failed extraction of results from the associated RPC calls as none were being returned. Adjustments to `getChainId()`, `getNonce()` and `CmdAssociateAddress()` were made NOTE: the latter `CmdAssociateAddress` had an internal cmd method call to `sei_associate` using GET instead of POST. Fixes REL-107
Refactored secret connection implementation, preparing it to multiplex between v2 and giga connections during handshake. In particular the handshake is now context aware. Additional changes: * removed gogo annotations from p2p protos * merged crypto/encoding package into crypto (for simplicity)
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v6.3 #2690 +/- ##
================================================
- Coverage 46.17% 42.02% -4.15%
================================================
Files 1171 1017 -154
Lines 101445 84792 -16653
================================================
- Hits 46841 35637 -11204
+ Misses 50510 45824 -4686
+ Partials 4094 3331 -763
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This test can fail flakily (eg [here](https://github.com/sei-protocol/sei-chain/actions/runs/20848553064/job/59897863084)) due to racy contention with `TestCoinTestSuite` (and maybe others). Note that the contention isn't lock-related, it's that the tests clobber the global value itself. I think requiring this test to be run serially is probably the best solution.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
I think we can bring this test back. I copied the buffered channel test logic from comm https://github.com/sei-protocol/sei-chain/blob/798a652a3e93fccb364b2952945183f2cc1be8a9/sei-tendermint/internal/consensus/common_test.go#L404
On mainnet/production setting the max gas wanted is set to 2x of max gas in genesis. Update testnet default settings to follow the same pattern in order to avoid false-positive issue reports and offer a more realistic testnet config. Fixes PLT-71
Encryption protocols need to send frames need to be of constant size (not too large to fit into IP packages, and not arbitrarily small to defend against message length analysis). As a result: * they isolate the user from network protocol overheads (no need to add extra buffering on top) * they should allow user to flush the buffers explicitly (otherwise frames would be underused)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe your changes and provide context
Testing performed to validate your change