In order to support having (plu)geth itself as a dependency, I had to implement some workarounds in plugeth/plugeth-utils:
The libsecp256k1 C library is present in plugeth-utils as well as geth, causing CGO to find duplicate symbols when linking, so I modified it to just link against geth's definitions instead of building when passed the build tag linkgeth.
Still need to verify whether this problem persists when using plugeth-utils/restricted/crypto/secp256k1 even without the geth dependency.
Due to a limitation with Go plugins, building a distributable plugin doesn't work if the main module is a dependency of the plugin, so I forked plugeth and added a nested proxy module (wrapmain) which just imports and runs the plugeth cmd package.
These (or at least 2) won't be needed once we refactor completely to plugeth types.
Next up:
Basic CI tests using stack-orchestrator
Refactor to completely use plugeth types, which will mean replicating chain generation and possibly upstreaming more changes.
Plugeth doesn't register service lifecycles. We can just Start the service when the plugin loads, but shutdown won't be handled cleanly. Should be easy enough to add alongside API registration.
Plugeth state methods are rudimentary, and won't allow caching - a new TrieDB object is created with every call. Will need to change this for good performance.
The Geth --log.vmodule flag for selective verbosity is not working for plugins, it's nice to have for debugging so we should fix it
Minimum viable implementation of statediff plugin.
#### Running:
* For building locally, refer to the "dev" targets in the Makefile on [this branch](https://git.vdb.to/cerc-io/plugeth-statediff/src/branch/dev-local-build).
* This uses a fork of plugeth, [see below](#plugeth-fork).
* Test with stack-orchestrator using [this branch](https://git.vdb.to/roysc/stack-orchestrator/src/branch/plugeth-testing). Note the containers need SSH keys configured to pull the Go dependencies from the Gitea server.
#### Summary of changes:
* Main refactors for plugeth integration
* Rename geth => plugeth types - kept to a minimum in this PR, so geth is still a dependency, more on that below
* Adds `main` package as plugin entrypoint
* Refactors flags to use stdlib `flag` package, for plugeth compatibility
* Refactors subscription endpoints to return channels and handle IDs internally
* When building state diff, store previous node blob to use as parent node
* Plugeth doesn't expose `Node()` lookup, so iterator parent path won't work
* Dropped `WithIntermediateStateNodes` from builder func names
* Removed unused metrics, cleaned up log and metrics calls
* Removes seemingly unused functions
* `ReadStorageReceipts`
* `WriteAncientBlock`
Changes we definitely need to upstream to `plugeth-utils` (see [dev branch](https://git.vdb.to/cerc-io/plugeth-utils/src/branch/statediff-additions)):
* Expose trie leaf and difference iterator
* Path encoding funcs
* Consensus encoding of receipts (`MarshalBinary`)
* add receipt EffectiveGasPrice (from [geth PR](https://github.com/ethereum/go-ethereum/issues/26713))
* Expose contract code
* Fix subscription endpoints ([done](https://github.com/openrelayxyz/plugeth/pull/78))
* Other random fixes
In order to support having (plu)geth itself as a dependency, I had to implement some workarounds in plugeth/plugeth-utils:
1. The `libsecp256k1` C library is present in plugeth-utils as well as geth, causing CGO to find duplicate symbols when linking, so I modified it to just link against geth's definitions instead of building when passed the build tag `linkgeth`.
* Still need to verify whether this problem persists when using `plugeth-utils/restricted/crypto/secp256k1` even without the geth dependency.
2. <a id="plugeth-fork"></a> Due to a [limitation with Go plugins](https://github.com/golang/go/issues/31354), building a distributable plugin doesn't work if the main module is a dependency of the plugin, so I [forked plugeth](https://git.vdb.to/cerc-io/plugeth/src/branch/statediff-wip) and added a nested proxy module (`wrapmain`) which just imports and runs the plugeth `cmd` package.
These (or at least 2) won't be needed once we refactor completely to plugeth types.
#### Next up:
* Basic CI tests using stack-orchestrator
* Refactor to completely use plugeth types, which will mean replicating chain generation and possibly upstreaming more changes.
* Plugeth doesn't register service lifecycles. We can just `Start` the service when the plugin loads, but shutdown won't be handled cleanly. Should be easy enough to add alongside API registration.
* Plugeth state methods are rudimentary, and won't allow caching - a new TrieDB object is created with every call. Will need to change this for good performance.
* The Geth `--log.vmodule` flag for selective verbosity is not working for plugins, it's nice to have for debugging so we should fix it
We had this previously because the statediffing process would fall/lag far enough behind head, below the pruning threshold of a full node, such that the full node would sometimes prune away the state before the statediffing service had used it. This shouldn't be a problem anymore after the multitude of improvements made to performance.
We had this previously because the statediffing process would fall/lag far enough behind head, below the pruning threshold of a full node, such that the full node would sometimes prune away the state before the statediffing service had used it. This shouldn't be a problem anymore after the multitude of improvements made to performance.
Nice, removing these unneccesary/redundant allocations and copys could have a noticeable impact on performance considering the number of times these operations are performed per block/statediff
Nice, removing these unneccesary/redundant allocations and `copy`s could have a noticeable impact on performance considering the number of times these operations are performed per block/statediff
I left it unchanged in the PR, but I would recommend a variable name other than `GITEA_TOKEN`, as I suspect that will be used by Gitea at some future date--like Github use `GITHUB_TOKEN`--and cause confusion since this is not the same token.
Something like `GIT_VDB_TO_TOKEN` might do, since we could follow the `HOST_NAME_TOKEN` pattern for other repos if needed.
Co-authored-by: Thomas E Lackey <telackey@bozemanpass.com>
Reviewed-on: #4
Co-authored-by: Thomas E Lackey <telackey@noreply.git.vdb.to>
Co-committed-by: Thomas E Lackey <telackey@noreply.git.vdb.to>
Right, and the tests are waiting on a way to provision access tokens on the runners so that image builds can access the Gitea server (@telackey is currently working on a solution).
Right, and the tests are waiting on a way to provision access tokens on the runners so that image builds can access the Gitea server (@telackey is currently working on a solution).
Right, and the tests are waiting on a way to provision access tokens on the runners so that image builds can access the Gitea server (@telackey is currently working on a solution).
> Right, and the tests are waiting on a way to provision access tokens on the runners so that image builds can access the Gitea server (@telackey is currently working on a solution).
I thought that problem was resolved?
(see: https://github.com/cerc-io/hosting/issues/49)
Might have been a miscommunication, as I wasn't attempting to create the token for use in CI as well (though I did test manually with a token created on my own account), since I do not have admin access to the organization or access to the cerccicd account.
That said, I am happy to do it. I just need the proper access.
Might have been a miscommunication, as I wasn't attempting to create the token for use in CI as well (though I did test manually with a token created on my own account), since I do not have admin access to the organization or access to the `cerccicd` account.
That said, I am happy to do it. I just need the proper access.
I made the token, ${{ secrets.CICD_REPO_TOKEN }} , and tweaked the plugeth and plugeth-statediff Dockerfiles / workflows to make use of it. More details are here: https://github.com/cerc-io/hosting/issues/49
While the actions still have other failures, they are getting past the Docker build.
I made the token, `${{ secrets.CICD_REPO_TOKEN }}` , and tweaked the plugeth and plugeth-statediff Dockerfiles / workflows to make use of it. More details are here: https://github.com/cerc-io/hosting/issues/49
While the actions still have other failures, they are getting past the Docker build.

The CI build now reaches the actual testing step (unit tests still have some edge cases that were already failing on the geth fork), so I will go ahead and merge this.
The CI build now reaches the actual testing step (unit tests still have some edge cases that were already failing on the geth fork), so I will go ahead and merge this.
roysc
merged commit 03f5622bea into main2023-07-14 06:08:08 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Minimum viable implementation of statediff plugin.
Running:
Summary of changes:
mainpackage as plugin entrypointflagpackage, for plugeth compatibilityNode()lookup, so iterator parent path won't workWithIntermediateStateNodesfrom builder func namesReadStorageReceiptsWriteAncientBlockChanges we definitely need to upstream to
plugeth-utils(see dev branch):MarshalBinary)In order to support having (plu)geth itself as a dependency, I had to implement some workarounds in plugeth/plugeth-utils:
libsecp256k1C library is present in plugeth-utils as well as geth, causing CGO to find duplicate symbols when linking, so I modified it to just link against geth's definitions instead of building when passed the build taglinkgeth.plugeth-utils/restricted/crypto/secp256k1even without the geth dependency.wrapmain) which just imports and runs the plugethcmdpackage.These (or at least 2) won't be needed once we refactor completely to plugeth types.
Next up:
Startthe service when the plugin loads, but shutdown won't be handled cleanly. Should be easy enough to add alongside API registration.--log.vmoduleflag for selective verbosity is not working for plugins, it's nice to have for debugging so we should fix it5214213cd6to60472965dfLooks great! A few comments but nothing major. Glad to see the overall structure of the code hasn't changed much despite the size of the diff.
@@ -258,0 +251,4 @@Our Postgres schemas are built around a single IPFS backing Postgres IPLD blockstore table(`ipld.blocks`) that conforms with[go-ds-sql](https://github.com/ipfs/go-ds-sql/blob/master/postgres/postgres.go). All IPLD objectsare stored in this table, where `key` is the blockstore-prefixed multihash key for the IPLD objectnitpick: in v5
keyis a full CID, not a blockstore-prefixed multihash@@ -30,3 +29,4 @@const APIName = "statediff"// APIVersion is the version of the state diffing service API// TODO: match package version?Not sure this makes sense since the package can be updated and version changed without affecting the API
@@ -57,2 +49,2 @@// create subscription and start waiting for eventsrpcSub := notifier.CreateSubscription()// Stream subscribes to statediff payloads as they are created.func (api *PublicAPI) Stream(ctx context.Context, params Params) (<-chan Payload, error) {Nice that they did away with the
*rpc.Subscription, returning a channel makes it much simpler@@ -113,2 +91,2 @@// create subscription and start waiting for eventsrpcSub := notifier.CreateSubscription()// StreamCodeAndCodeHash writes all of the codehash=>code pairs at a given block to a websocket channel.func (api *PublicAPI) StreamCodeAndCodeHash(ctx context.Context, blockNumber uint64) (<-chan types.CodeAndCodeHash, error) {We haven't ever actually used this endpoint, should consider getting rid of it.
In that case, I vote for dropping it for now, pending a use case
That sounds good to me.
@@ -0,0 +25,4 @@GetBlockByNumber(number uint64) *types.BlockGetReceiptsByHash(hash common.Hash) types.ReceiptsGetTd(hash common.Hash, number uint64) *big.Int// TODO LockTrie is never usedWe had this previously because the statediffing process would fall/lag far enough behind head, below the pruning threshold of a full node, such that the full node would sometimes prune away the state before the statediffing service had used it. This shouldn't be a problem anymore after the multitude of improvements made to performance.
@@ -0,0 +50,4 @@for event := range bufferChan {block := utils.MustDecode[types.Block](event.Block)// TODO: apparently we ignore the logs// logs := utils.MustDecode[types.Log](chainEvent.Logs)We should get the logs when we lookup the Receipts and pack them into the statediff payload.
@@ -286,3 +266,2 @@func (sdb *StateDiffBuilder) processStateValueNode(it trie.NodeIterator, parentBlob []byte) (*sdtypes.AccountWrapper, error) {var account types.StateAccountaccountRLP := make([]byte, len(it.LeafBlob()))copy(accountRLP, it.LeafBlob())Nice, removing these unneccesary/redundant allocations and
copys could have a noticeable impact on performance considering the number of times these operations are performed per block/statediff@@ -81,6 +81,31 @@ func processReceiptsAndLogs(rcts []*types.Receipt) ([]*EthReceipt, [][]*EthLog,return ethRctNodes, ethLogNodes, nil}// // processReceiptsAndLogs will take in receiptsWhat's this commented out stuff for?
for disposal, just left over from checking something
@@ -0,0 +25,4 @@func Initialize(ctx core.Context, pl core.PluginLoader, logger core.Logger) {log.SetDefaultLogger(logger)// lvl, err := strconv.ParseInt(ctx.String("verbosity"), 10, 8)Is this from geth upstream or here for future use? If not, lets remove the commented out stuff.
no, just left over from debugging logging
969f9c3b14to52debaa75852debaa758to9b937d90712d6cb835e2to76b8359beab02435eee9to0b9637b4e916c7b262c5todee59f4e76Hey @roysc are we waiting for the integration testing to be complete before merging this?
Right, and the tests are waiting on a way to provision access tokens on the runners so that image builds can access the Gitea server (@telackey is currently working on a solution).
I thought that problem was resolved?
(see: https://github.com/cerc-io/hosting/issues/49)
That lets us use a token to pull packages into the image, but it was my understanding that there is still no token available to use on the runner (?)
Might have been a miscommunication, as I wasn't attempting to create the token for use in CI as well (though I did test manually with a token created on my own account), since I do not have admin access to the organization or access to the
cerccicdaccount.That said, I am happy to do it. I just need the proper access.
I made the token,
${{ secrets.CICD_REPO_TOKEN }}, and tweaked the plugeth and plugeth-statediff Dockerfiles / workflows to make use of it. More details are here: https://github.com/cerc-io/hosting/issues/49While the actions still have other failures, they are getting past the Docker build.
08b9a9783ato413d6afd384cc5eef23etoedde732a1cedde732a1cto03f5622beaThe CI build now reaches the actual testing step (unit tests still have some edge cases that were already failing on the geth fork), so I will go ahead and merge this.