fix(dot/types): *types.Body to be of type []types.Extrinsic #1807

kishansagathiya · 2021-09-24T13:07:16Z

Changes

Changing Body from []byte to []Extrinsic, changes its meaning (encoded to not encoded), so the right way to do this should be as follows

Create a new Struct BodyExtrinsics with its own methods
Use BodyExtrinsics instead of Body everywhere and changes/use methods accordingly
Now that old Body struct is not being used anywhere, remove it.
Rename BodyExtrinsics to Body and accordingly rename its methods

Tests

Issues

Closes update *types.Body to be of type []types.Extrinsic #1775

Primary Reviewer

CLAassistant · 2021-09-24T13:07:30Z

All committers have signed the CLA.

dot/types/body.go

Create a new BodyExtrinsics struct with its own methods and use it instead of Body

Rename BodyExtrinsics to Body and accordingly rename its methods

codecov · 2021-09-28T15:17:34Z

Codecov Report

Merging #1807 (760b2af) into development (5c11fb2) will increase coverage by 0.07%.
The diff coverage is 80.00%.

@@               Coverage Diff               @@
##           development    #1807      +/-   ##
===============================================
+ Coverage        59.62%   59.69%   +0.07%     
===============================================
  Files              188      188              
  Lines            19686    19669      -17     
===============================================
+ Hits             11738    11742       +4     
+ Misses            5966     5954      -12     
+ Partials          1982     1973       -9

Flag	Coverage Δ
unit-tests	`59.69% <80.00%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dot/rpc/modules/author.go	`72.58% <ø> (ø)`
dot/types/block_data.go	`6.25% <ø> (ø)`
dot/state/block.go	`41.55% <57.14%> (-0.09%)`	⬇️
dot/types/block.go	`68.00% <66.66%> (-7.00%)`	⬇️
dot/types/body.go	`54.54% <70.83%> (+3.75%)`	⬆️
dot/core/service.go	`53.63% <100.00%> (+0.85%)`	⬆️
dot/network/test_helpers.go	`83.00% <100.00%> (+0.17%)`	⬆️
dot/rpc/modules/childstate.go	`91.48% <100.00%> (+3.61%)`	⬆️
dot/state/test_helpers.go	`63.77% <100.00%> (+0.57%)`	⬆️
dot/sync/syncer.go	`46.83% <100.00%> (+0.77%)`	⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a04deb6...760b2af. Read the comment docs.

dot/types/body_extrinsics.go

arijitAD · 2021-09-28T15:24:35Z

dot/types/body_extrinsics.go

-func NewBody(b []byte) *Body {
-	body := Body(b)
+// NewBody returns a BodyExtrinsics from an Extrinsic array.
+func NewBody(e []Extrinsic) *Body {


Any reason for the body to be a pointer? Since it is already a slice.

I have kept what was being used previously.
Not using pointer would be safer in general. Changing this from pointer, would mean edit this at a lot places. But, I think doing so would be the right way. Will change it.

At most place where body is returned it is returned as a pointer.

So if passed without pointer to the slice (Body and not *Body), you can modify it, but to an extent. From what I read, you can modify, values but not length of the slice. If you try to append to it, slice won't have values that you expected. So far, I have not seen anywhere Body getting modified.

I also see that at places a pointer to Body and a pointer is stored for even other slices.

type BlockData struct { Hash common.Hash Header *Header Body *Body Receipt *[]byte MessageQueue *[]byte Justification *[]byte }

@noot is returning or storing *Body (and also other slices, like MessageQueue *[]byte) done deliberately? Should I care returning Body instead of *Body?

all the fields in the BlockData should be a pointer as they are optional types. please leave this as is

actually, for this functions specifically, I'm not sure if it really matters. I think it could be either

dot/types/body_extrinsics.go

dot/state/block.go

dot/types/block.go

dot/types/body_extrinsics.go

noot

looks good! just a few comments

dot/core/service_test.go

dot/types/body.go

dot/rpc/modules/chain_test.go

dot/types/body.go

noot · 2021-10-01T15:07:54Z

@kishansagathiya could you resolve the conflicts when you get a chance?

noot · 2021-10-05T09:50:38Z

dot/types/body_test.go

 	}
+
+	fmt.Println(extStrings)


@kishansagathiya can you remove this?

noot

nice work :D

timwu20 · 2021-10-06T14:19:41Z

dot/types/block.go

+	if err != nil {
+		return nil, err
+	}
+	return append(enc, encodedBody...), nil


I think this whole function can be return scale.Marshal(*b)

timwu20 · 2021-10-06T14:30:13Z

dot/types/body.go

+	var err error
+
+	for i, ext := range decodedExts {
+		ret[i], err = scale.Marshal(ext)


if []Extrinsic is not supposed to be encoded, why are we marshalling here? I see that the old code did this, but should this not return [][]byte given that we are using the Extrinsic type to represent the decoded extrinsic? @noot @kishansagathiya

The marshaling part is correct, but I think this function should be better named as AsEncodedBytesArray. That would avoid the confusion that it is causing. And the value returned by this function is being converted into [][]byte and []string anyway, so returning [][]byte is better

Are we sure that the marshalling here is correct? On my branch jimmy/integrateScaleIntoTypes I had my own tests

gossamer/dot/types/block_test.go

Line 111 in 0cb69f5

func TestMustEncodeBlock(t *testing.T) {

for block encoding written which I compare the encoding to the expected byte string (hard coded so I don't test against itself) and when I merge these changes that test fails because the encoding doesn't equal what is expected. So there appears to be difference between the encodings, but I am not certain what the correct format is. For example, the body.AsEncodedExtrinsics() function specifies that it is scale encoded but that is not actually the case because there is not length prefix for the encoded slice. I am not positive if it should be scale encoded or if its just incorrect documentation but it does appear that the body encoding now is different than it was before. Perhaps @noot could help provide some clarification on this?

# [0.6.0](v0.5.0...v0.6.0) (2021-12-03) ### Bug Fixes * **babe:** Fix extrinsic format in block. ([#1530](#1530)) ([1a03b2a](1a03b2a)) * **ci:** add missing go-version matrix to fix development branch CI ([#2037](#2037)) ([6babe76](6babe76)) * **cmd/cfg:** Use Babe Lead value from toml config ([#2032](#2032)) ([06aa3e3](06aa3e3)) * cmd/gossamer: Generate random name if --name flag not set ([#1506](#1506)) ([3c05a88](3c05a88)) * confirm block import notifier is closed properly ([#1736](#1736)) ([ad2d85e](ad2d85e)) * **docs:** improve build-spec usage docs ([#1706](#1706)) ([2e164b4](2e164b4)) * **dot/core:** Add only extrinsic during chain reorg. ([#1609](#1609)) ([29413d4](29413d4)) * **dot/core:** Batch process transaction message. ([#1780](#1780)) ([0064836](0064836)) * **dot/core:** check transaction Validity.Propagate field to determine whether to propagate tx ([#1643](#1643)) ([81f23cc](81f23cc)) * **dot/core:** Fix handle transaction message test. ([#1607](#1607)) ([58b8725](58b8725)) * **dot/network, lib/grandpa:** fix handshake decoding and grandpa message handler sigabort ([#1631](#1631)) ([887f72c](887f72c)) * **dot/network, lib/grandpa:** fix node sync, improve devnet finality ([bcc7935](bcc7935)) * **dot/network:** add nil checks in connManager ([#2069](#2069)) ([7f9c042](7f9c042)) * **dot/network:** Check for size when decoding leb128. ([#1634](#1634)) ([d082b9e](d082b9e)) * **dot/network:** check if peer supports protocol ([#1617](#1617)) ([6bf66a4](6bf66a4)) * **dot/network:** decrease DHT find peers interval for gssmr nodes ([#1703](#1703)) ([08516a0](08516a0)) * **dot/network:** fix bugs in notifications protocol handlers; add metrics for inbound/outbound streams ([#2010](#2010)) ([8c2993d](8c2993d)) * **dot/network:** fix dht connection on discovery on devnet ([#2059](#2059)) ([da065b8](da065b8)) * **dot/network:** fix discovery between gossamer nodes ([#1594](#1594)) ([f4c79d3](f4c79d3)) * **dot/network:** fix justification request at head logic ([#1510](#1510)) ([98d1413](98d1413)) * **dot/network:** fix memory allocations with `sizedBufferPool` ([#1963](#1963)) ([e0b126b](e0b126b)) * **dot/network:** Fix missing digest in header ([#2092](#2092)) ([21ea85e](21ea85e)) * **dot/network:** Fix notification handshake and reuse stream. ([#1545](#1545)) ([a632dc4](a632dc4)) * **dot/network:** fix receiving notifications messages from substrate peers ([#1517](#1517)) ([fdf3c53](fdf3c53)) * **dot/network:** fix stream manager tests ([#1683](#1683)) ([e02eca4](e02eca4)) * **dot/network:** implement a handshake timeout ([#1615](#1615)) ([87c2f63](87c2f63)) * **dot/network:** Implement time based handle transaction ([#1942](#1942)) ([dd08424](dd08424)) * **dot/network:** move low reputation peer removal from network ConnManager to peer scoring logic (dot/peerstate) ([#2068](#2068)) ([ac16285](ac16285)), closes [#2039](#2039) * **dot/network:** Return on EOF error while reading stream. ([#1733](#1733)) ([f447eac](f447eac)) * **dot/network:** split stored streams and handshakeData into inbound and outbound ([#1553](#1553)) ([637050b](637050b)) * **dot/network:** update notificationsProtocol handshakeData to sync.Map ([#1492](#1492)) ([22f7269](22f7269)) * **dot/node:** Start websocket server only with `--ws` flag ([#1671](#1671)) ([6ecef3b](6ecef3b)) * **dot/state, lib/babe, lib/trie:** improve syncing between gossamer authority nodes ([#1613](#1613)) ([ca99fbf](ca99fbf)) * **dot/state, lib/grandpa:** update justification and SignedVote handling in database ([#1682](#1682)) ([bbdcd6f](bbdcd6f)) * **dot/state,dot/network:** improve memory usage when syncing ([#1491](#1491)) ([3b2ad8d](3b2ad8d)) * **dot/state:** add StorageState Lock/Unlock API for usage by babe and sync ([#1700](#1700)) ([3c22ace](3c22ace)) * **dot/state:** fix deadlock, fixes bootstrap syncing ([#1959](#1959)) ([dd80c09](dd80c09)) * **dot/state:** fix usage of trie.Snapshot ([#1489](#1489)) ([3880a40](3880a40)) * **dot/state:** track runtime per-block, fix runtime upgrades differing between forks ([#1638](#1638)) ([e133884](e133884)) * **dot/state:** update `*state.BlockState.AddBlockToBlockTree` to store block in `unfinalisedBlocksMap` ([#2006](#2006)) ([55d997f](55d997f)) * **dot/sync:** add nil header checks ([#2099](#2099)) ([a7d4be0](a7d4be0)) * **dot/sync:** fix block request and response logic ([#1907](#1907)) ([9c6283e](9c6283e)) * **dot/sync:** fix creating block response, fixes node sync between gossamer nodes ([#1572](#1572)) ([1328c80](1328c80)) * **dot/telemetry:** refactor telemetry to reduce CPU usage ([#1597](#1597)) ([bc31ac7](bc31ac7)) * **dot/types:** *types.Body to be of type []types.Extrinsic ([#1807](#1807)) ([4c09715](4c09715)) * **dot/types:** fix max value for digest ([#1687](#1687)) ([48405e7](48405e7)) * **dot:** fix `TestNewNode` ([#2070](#2070)) ([42908d0](42908d0)) * fix edit link ([#1507](#1507)) ([5089327](5089327)) * fix Kusama sync; add storageState lock in core.HandleTransactionMessage ([#1783](#1783)) ([1d688e4](1d688e4)) * **lib/babe, lib/runtime/wasmer:** fixes for v0.9.8+ runtime ([#2075](#2075)) ([2f9f80c](2f9f80c)) * **lib/babe:** add `--babe-lead` flag, update epoch handling logic ([#1895](#1895)) ([7abcce6](7abcce6)) * **lib/babe:** add pre-runtime digest before calling initialize_block ([#1581](#1581)) ([c1b26d3](c1b26d3)) * **lib/babe:** always use 2/3 of slot to produce block, re-add potentially valid txs to queue ([#1679](#1679)) ([cf93ad3](cf93ad3)) * **lib/babe:** call AddBlock in BABE synchronously ([#1585](#1585)) ([86acc43](86acc43)) * **lib/babe:** fix BABE state storing after building block ([#1536](#1536)) ([1a3dea2](1a3dea2)) * **lib/babe:** fix err log ([#1801](#1801)) ([a96f06a](a96f06a)) * **lib/babe:** fix setting first slot of network, fix loading BABE epoch params ([#1640](#1640)) ([5c3dbfe](5c3dbfe)) * **lib/babe:** fix timing for transition between epochs ([#1636](#1636)) ([57027db](57027db)) * **lib/blocktree:** fix blocktree bug ([#2060](#2060)) ([c17b53a](c17b53a)) * **lib/blocktree:** fix potential nil pointer dereference in `HighestCommonAncestor`, core `handleBlocksAsync` ([#1993](#1993)) ([f7f4463](f7f4463)) * **lib/blocktree:** fix setting leaves after blocktree pruning ([#1605](#1605)) ([58c0854](58c0854)) * **lib/blocktree:** removes the inconsistency to choose a deepest leaf ([#2094](#2094)) ([43d68e3](43d68e3)) * **lib/crypto/ed25519:** update ed25519 to use go-schnorrkel bip39 derivation ([#1488](#1488)) ([dfb95d2](dfb95d2)) * **lib/genesis:** Update missing and incorrect fields in genesis file. ([#1681](#1681)) ([8207704](8207704)) * **lib/grandpa:** fix grandpa stall and various bugs ([#1708](#1708)) ([67c93f4](67c93f4)) * **lib/grandpa:** fix grandpa vote message switch ([#2095](#2095)) ([461890c](461890c)) * **lib/grandpa:** fix threshold checking to be strictly greater than 2/3 ([#1891](#1891)) ([66ffe51](66ffe51)) * **lib/grandpa:** use `defaultGrandpaInterval` if not set, fixes error on startup ([#1982](#1982)) ([75627b5](75627b5)) * **lib/runtime/life:** remove import C from life ([#1923](#1923)) ([ed507d2](ed507d2)) * **lib/runtime:** update HOST_API_TEST_RUNTIME_URL to reference specific commit ([#1885](#1885)) ([666ed06](666ed06)) * **log-levels:** do not ignore configuration file log levels ([#2016](#2016)) ([80879b2](80879b2)) * pending bubble hidden after block included ([#1592](#1592)) ([5826322](5826322)) * persist node name ([#1543](#1543)) ([88b88f2](88b88f2)) * **pprof:** only run pprof service if enabled ([#2073](#2073)) ([55669c5](55669c5)) * **release:** Trigger release when pushed to main branch. ([#1566](#1566)) ([d445c97](d445c97)) * **rpc/subscription:** subscribe runtime version notify when version changes ([#1686](#1686)) ([9a76d39](9a76d39)) * Staging CI workflow ([#2034](#2034)) ([84ec792](84ec792)) * **trie:** memory leak fix in `lib/trie` ([#2009](#2009)) ([0ad5eb7](0ad5eb7)) * update deprecated package ([#1603](#1603)) ([f195204](f195204)) * update go-schnorrkel version ([#1557](#1557)) ([b86c7ff](b86c7ff)) * update gssmr genesis to use pallet_babe::SameAuthoritiesForever ([#1696](#1696)) ([fb0a751](fb0a751)) * update HOST_API_TEST_RUNTIME_URL ([#1898](#1898)) ([2ef59a8](2ef59a8)) * **utils:** create a specific folder for database ([#1598](#1598)) ([8c67795](8c67795)) ### Features * add --chain dev option ([#1561](#1561)) ([04a2969](04a2969)) * Add properties and chainId on build-spec command ([#1520](#1520)) ([b18290c](b18290c)) * cmd: implement import-runtime subcommand ([#1483](#1483)) ([d82b2da](d82b2da)) * **cmd/gossamer:** implement --telemetry-url parameter ([#1890](#1890)) ([b202e89](b202e89)), closes [#1502](#1502) * **cmd:** implement offline pruning of state trie ([#1564](#1564)) ([af9c925](af9c925)) * **devnet:** Local Gossamer Devnet ([#2008](#2008)) ([a520001](a520001)) * **dot/network, lib/grandpa:** request justification on receiving NeighbourMessage, verify justification on receipt ([#1529](#1529)) ([e1f9f42](e1f9f42)) * **dot/network:** Add cache for network message. ([#1511](#1511)) ([accaf69](accaf69)) * **dot/network:** add propagate return bool to messageHandler func type to determine whether to propagate message or not ([#1555](#1555)) ([0d6f488](0d6f488)) * **dot/network:** implement persistent peers functionality ([#1512](#1512)) ([7850532](7850532)) * **dot/network:** implement streamManager to cleanup not recently used streams ([#1611](#1611)) ([ba861bf](ba861bf)) * **dot/network:** request block justifications when near head ([#1499](#1499)) ([ae7012b](ae7012b)) * **dot/peerset:** Implement peer scoring ([#1791](#1791)) ([1c989ad](1c989ad)) * **dot/rpc/modules:** add `system_addReservedPeer` and `system_removeReservedPeer` RPC call ([#1712](#1712)) ([dba5922](dba5922)) * **dot/rpc:** Add `system_localListenAddresses` RPC call ([#1689](#1689)) ([c981d2e](c981d2e)) * **dot/rpc:** Implement `childstate_getKeys` rpc call ([#1800](#1800)) ([9b2f41e](9b2f41e)) * **dot/rpc:** implement sync_state_genSyncSpec RPC call ([#1827](#1827)) ([2186caf](2186caf)) * **dot/state:** implement online pruning of historical state tries ([#1596](#1596)) ([3eb9399](3eb9399)) * **dot/sync:** implement codeSubstitutes ([#1635](#1635)) ([d87aaeb](d87aaeb)) * dot/telemetry: Implement basic telemetry connection ([#1497](#1497)) ([fcb4159](fcb4159)) * **dot/telemetry:** Added connection retry ([#1904](#1904)) ([579a791](579a791)) * **dot/telemetry:** Added more telemetry messages in grandpa client ([#2043](#2043)) ([2e57d15](2e57d15)), closes [#1841](#1841) [#1842](#1842) * **dot/telemetry:** implement notify.finalized telemetry interface ([#1877](#1877)) ([de1a60d](de1a60d)) * **dot/telemetry:** implement substrate_number_leaves metrics ([#1926](#1926)) ([69823c0](69823c0)) * **dot/telemetry:** implement telemetry message network_state ([#1618](#1618)) ([a81844e](a81844e)) * **flags:** read log levels from flags ([#1953](#1953)) ([9694e46](9694e46)) * implement ext_default_child_storage_storage_kill_version_2 ([#1799](#1799)) ([c2908ae](c2908ae)) * implement ext_offchain_index_set_version_1 for wasmer runtime ([#1739](#1739)) ([96c30a6](96c30a6)) * **lib/babe:** add check of types.ConfigData.SecondarySlots for disabling secondary verification ([#1910](#1910)) ([cd27ae4](cd27ae4)) * **lib/grandpa:** fully verify justifications using GrandpaState ([#1544](#1544)) ([028d25e](028d25e)) * **lib/grandpa:** Include equivocatory nodes while creating justification ([#1911](#1911)) ([aca86b6](aca86b6)) * **lib/grandpa:** send NeighbourMessage to peers ([#1558](#1558)) ([322ccf9](322ccf9)) * **lib/runtime/wasmer:** implement ext_default_child_storage_storage_kill_version_3 ([#1878](#1878)) ([a719a60](a719a60)) * **lib/runtime/wasmer:** implement ext_offchain_local_storage_version_1 ([#1821](#1821)) ([0f63b17](0f63b17)) * **lib/runtime:** Implement `ext_offchain_http_request_add_header_version_1` host function ([#1994](#1994)) ([0a30b3d](0a30b3d)) * **lib/runtime:** Implement `ext_offchain_http_request_start_version_1` host function ([#1947](#1947)) ([974b1fc](974b1fc)) * **lib/runtime:** Implement `trie_blake2_256_verify_proof` host function ([#1920](#1920)) ([506565d](506565d)) * **lib/trie:** Implement `verify_proof` function ([#1883](#1883)) ([67bb5ef](67bb5ef)) * **lib/trie:** Implement limit for trie.ClearPrefix ([#1905](#1905)) ([becec9e](becec9e)) * **lib/trie:** Parallel hash trie. ([#1657](#1657)) ([22827e7](22827e7)) * **pprof:** Pprof HTTP server service ([#1991](#1991)) ([ce24ea9](ce24ea9)) * **rpc/subscription:** implement state_unsubscribeStorage ([#1574](#1574)) ([7574f10](7574f10)) * **rpc:** Implement `childstate_getChildStorage` RPC call ([#1832](#1832)) ([3d949f2](3d949f2)) * **rpc:** Implement `childstate_getStorageHash` RPC call ([#1805](#1805)) ([e539bd3](e539bd3)) * **rpc:** Implement `childstate_getStorageSize` RPC call ([#1810](#1810)) ([a04deb6](a04deb6)) * **rpc:** Implement `payment_queryInfo` RPC call ([#1826](#1826)) ([7a5deec](7a5deec)) * **rpc:** Implement `state_getReadProof` rpc call ([#1768](#1768)) ([865f80f](865f80f)) * **runtime:** implement custom logging handler that print function name ([#1825](#1825)) ([2b1276d](2b1276d)) * **telemetry:** send telemetry messages when GRANDPA receieves commit or vote messages ([#2015](#2015)) ([7bf40e1](7bf40e1)), closes [#1840](#1840) [#1839](#1839) [#1838](#1838) * **telemetry:** send txpool.import telemetry msg ([#1966](#1966)) ([ffc81bf](ffc81bf)) ### Reverts * Revert "feat(dot/rpc) implement `author_hasSessionKeys` RPC call (#1704)" (#1714) ([65380fd](65380fd)), closes [#1704](#1704) [#1714](#1714)

github-actions · 2021-12-03T21:09:23Z

🎉 This PR is included in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

…e#1807)

noot reviewed Sep 25, 2021

View reviewed changes

dot/types/body.go Outdated Show resolved Hide resolved

BodyExtrinsics instead of Body

9dd327a

Create a new BodyExtrinsics struct with its own methods and use it instead of Body

kishansagathiya force-pushed the issue-1775 branch from 1224d38 to 9dd327a Compare September 28, 2021 08:43

kishansagathiya added 2 commits September 28, 2021 14:58

Merge branch 'development' into issue-1775

6cb67a8

Tests for BodyExtrinsics, Removed old Body struct

70cd6d1

kishansagathiya changed the title ~~WIP *types.Body to be of type []types.Extrinsic~~ *types.Body to be of type []types.Extrinsic Sep 28, 2021

kishansagathiya marked this pull request as ready for review September 28, 2021 12:00

kishansagathiya requested review from arijitAD, EclesioMeloJunior, edwardmack, jimjbrettj and timwu20 as code owners September 28, 2021 12:00

Rename BodyExtrinsics to Body

51cbe83

Rename BodyExtrinsics to Body and accordingly rename its methods

noot self-requested a review September 28, 2021 14:19

arijitAD reviewed Sep 28, 2021

View reviewed changes