Swarm network rewrite ethereum main repo relevant changes by acud · Pull Request #654 · ethersphere/swarm

acud · 2018-05-30T12:33:32Z

No description provided.

nonsense · 2018-05-30T12:41:34Z

 var prevTime time.Time
 var cntPrev int

-func TestSimulation(t *testing.T) {


@justelad we have to revert this on our end. Whisper team most probably has already fixed this.

nonsense · 2018-05-30T12:41:48Z


 		err = node.server.Start()
 		if err != nil {
-			t.Fatalf("failed to start server %d.", i)


@justelad we have to revert this as well.

nonsense · 2018-05-30T12:42:41Z

 }

+// GetNodes returns the existing nodes that are up
+func (net *Network) GetUpNodes() (nodes []*Node) {


I asked about this method a few weeks ago, and it is still not used. I think we should not be committing dead code.

acud · 2018-05-30T12:48:46Z

i think that travis will fail bluntly on the cmd/swarm section. it depends on stuff which needs to be merged from the swarm package. will have to shelve that too with the main merge

nonsense · 2018-05-30T13:02:35Z

 )

 func TestUPNP_DDWRT(t *testing.T) {
+	t.Skip("broken")


This is passing on master, not sure why we are skipping it.

nonsense · 2018-05-30T13:04:31Z

+}
+
+// tcpPipe creates an in process full duplex pipe based on a localhost TCP socket
+func tcpPipe() (net.Conn, net.Conn, error) {


This must go away.

@nonsense go away as in go away with all of the code which depends on it? there's dependant code in pss_test and snapshot_sync_test

We already have this in rlpx_test.go - we should export TCPPipe and reuse in this package.

acud · 2018-05-30T13:05:59Z

 	configs := []string{
 		"--vendor",
 		"--tests",
+		"--deadline=10m",


@nonsense do we want to introduce this?

Yes, I think there is no harm in it.

acud · 2018-05-30T13:07:38Z

 		Msg:  msg,
 		Ctx:  newContext(l.ctx, ctx),
-		Call: stack.Caller(2),
+		Call: stack.Caller(skip),


@nonsense why was this change introduced? i mean, not better to have this adjustable from one spot instead of 12 others?

This is needed, so that we have the Output method, which takes a different skip level.

See function Respond in swarm/api/http/error.go

ok, but maybe we could have a const for the default value of 2?

why do we need this change?

@zelig it is explained above why we need it - See function Respond and the Output method. This helps us track the caller line more stacks up, than just 2.

acud · 2018-05-31T12:00:57Z

@nonsense this is ready for another review

zelig · 2018-06-02T07:28:01Z


 /**
- * Transforms given string to valid 20 bytes-length addres with 0x prefix
+ * Transforms given string to valid 20 bytes-length address with 0x prefix


this change should be sumbitted to web3 js not here this file is updated from there.
Skip this one

zelig · 2018-06-02T07:28:58Z

 const (
 	timeFormat     = "2006-01-02T15:04:05-0700"
-	termTimeFormat = "01-02|15:04:05"
+	termTimeFormat = "01-02|15:04:05.999999"


do we really need nanosecond resolution on log stamps?

With all the race conditions we had to track and fix, this was very useful.

zelig · 2018-06-02T07:29:40Z

 		Msg:  msg,
 		Ctx:  newContext(l.ctx, ctx),
-		Call: stack.Caller(2),
+		Call: stack.Caller(skip),


why do we need this change?

zelig · 2018-06-02T07:38:04Z

 	errc := make(chan error)

 	go func() {
+		log.Trace(fmt.Sprintf("trigger %v (%v)....", trig.Msg, trig.Code))


remove these trace

zelig · 2018-06-02T07:38:48Z

 	// shrinks on demand. If the buffer reaches the size below, the subscription is
 	// dropped.
-	maxClientSubscriptionBuffer = 8000
+	maxClientSubscriptionBuffer = 20000


please mention this is the PR synopsis as separate point

nonsense · 2018-06-04T10:28:04Z

@zelig why do we need this change? -> because of the Output method, which is used for logging errors - we care about the line of the Error, and not about the line that logs the Error.

nonsense · 2018-06-04T10:31:27Z

+// You should have received a copy of the GNU Lesser General Public License
+// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.
+
+package helpers


this is a bad name for the package. maybe just call it pipes?

nonsense · 2018-06-04T10:33:56Z

@justelad thanks for taking care of this.

Just two remarks from my side:

Rename helpers package to pipes.
Remove the tracing that @zelig noted.

The rest I am happy with and hopefully will get approved on ethereum/go-ethereum side.

acud · 2018-06-04T11:22:49Z

@zelig @nonsense your comments were addressed. can i reopen this on the main repo?
last delta: 9edebd8

…all non-swarm package changes relevant for the main go-ethereum repository for the upcoming swarm-network-rewrite merge into master rpc/client: increased maxClientSubscriptionBuffer to 20000

Fix a spelling mistake in comment

…f bool (#16757) Fixes #16756

This commit adds many comments and removes unused code. It also removes the EmptyHash function, which had some uses but was silly.

…t (#16369)

This removes a golint warning: type name will be used as trie.TrieSync by other packages, and that stutters; consider calling this Sync. In hexToKeybytes len(hex) is even and (even+1)/2 == even/2, remove the +1.

acud requested review from fjl, gluk256 and zsfelfoldi as code owners May 30, 2018 12:33

nonsense removed request for fjl, gluk256 and zsfelfoldi May 30, 2018 12:38

nonsense reviewed May 30, 2018

View reviewed changes

acud commented May 30, 2018

View reviewed changes

acud added the ready for another review label May 31, 2018

zelig suggested changes Jun 2, 2018

View reviewed changes

nonsense reviewed Jun 4, 2018

View reviewed changes

nonsense approved these changes Jun 4, 2018

View reviewed changes

karalabe and others added 9 commits June 4, 2018 14:52

trie: cleaner logic, one less func call

b0b6835

eth, node, trie: fix minor typos (#16802)

1859e31

build, cmd, common, contracts, internal, log, metrics, p2p, whisper: …

c543057

…all non-swarm package changes relevant for the main go-ethereum repository for the upcoming swarm-network-rewrite merge into master rpc/client: increased maxClientSubscriptionBuffer to 20000

params: release go-ethereum v1.8.9

03bd293

VERSION, params: begin 1.8.10 release cycle

dfac8d2

ethereum: fix a typo in FilterQuery{} (#16827)

6a48887

Fix a spelling mistake in comment

eth/fetcher: reuse variables for hash and number (#16819)

fe7694f

whisper/shhclient: update call to shh_post to expect string instead o…

a12ffb3

…f bool (#16757) Fixes #16756

common: improve documentation comments (#16701)

463ff93

This commit adds many comments and removes unused code. It also removes the EmptyHash function, which had some uses but was silly.

dshulyak and others added 8 commits June 4, 2018 14:56

p2p/discv5: add egress/ingress traffic metrics to discv5 udp transpor…

9f8f2e7

…t (#16369)

core: improve test for TransactionPriceNonceSort (#16413)

49dbbcf

core/vm: fix typo in comment

c6a64ec

trie: rename TrieSync to Sync and improve hexToKeybytes (#16804)

43ee906

This removes a golint warning: type name will be used as trie.TrieSync by other packages, and that stutters; consider calling this Sync. In hexToKeybytes len(hex) is even and (even+1)/2 == even/2, remove the +1.

core: fix transaction event asynchronicity

9d61b3f

params: release Geth 1.8.10 hotfix

e20801a

VERSION, params: begin 1.8.11 release cycle

300a5f8

ethstats: fix last golint warning (#16837)

0c7f221

acud force-pushed the swarm-network-rewrite-eth-relevant-changes branch from e50646c to 0c7f221 Compare June 4, 2018 12:57

acud closed this Jun 4, 2018

This was referenced Jun 4, 2018

build, common, contracts, log, metrics, p2p, rpc: changes necessary for swarm network rewrite merge ethereum/go-ethereum#16886

Closed

build, contracts, log, metrics, p2p, rpc: changes necessary by swarm-network-rewrite ethereum/go-ethereum#16898

Merged

Conversation

acud commented May 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acud commented May 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acud commented May 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nonsense commented Jun 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nonsense commented Jun 4, 2018

Uh oh!

acud commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

acud commented Jun 4, 2018 •

edited

Loading