client/eth: Allow harness testing on testnet. #1550
Conversation
JoeGruffins
force-pushed
the
ethtestontestnet
branch
from
2b369f6
to
6085aea
Compare
| if prog.CurrentBlock >= prog.HighestBlock { | ||
| return nil | ||
| if testnet { | ||
| syncing := prog.CurrentBlock > prog.HighestBlock || prog.CurrentBlock == 0 |
There was a problem hiding this comment.
I'm confused by prog.CurrentBlock > prog.HighestBlock here. For simnet we declare synced (not syncing) as prog.CurrentBlock >= prog.HighestBlock && prog.CurrentBlock != 0.
Should this here be a < to indicate syncing?
There was a problem hiding this comment.
I'm also a little confused here. I beleive HighestBlock is our local synced block, and CurrentBlock is the best block from the perspective of a peer. CurrentBlock always starts at zero I believe, when we have no peers. This is correct for syncing I think, after a bit of observing sure seems that way. If current block has reached highest block (==), we are synced and no longer syncing. We are syncing as long as current/best is larger and not equal to highest/local tip. I keep looking at SyncStatus thinking somethings wrong but I don't want to touch it in this pr.
Looking again, the simnet version should just be == as we want to return when synced. Also we don't want to worry about the header being old there.
I think that the previous check actually caused an error on simnet for the first time testing, because current and highest would both be zero in the beginning, returning before any sync had started.
There was a problem hiding this comment.
I beleive HighestBlock is our local synced block, and CurrentBlock is the best block from the perspective of a peer
Pretty sure it's the opposite. https://ethereum.stackexchange.com/a/424/90199
There was a problem hiding this comment.
With fmt.Printf("client: %d\ncurrent: %d\nhighest: %d\nstarting: %d\n", clientNum, prog.CurrentBlock, prog.HighestBlock, prog.StartingBlock)
See:
Testnet nodes starting sync, this may take a while...
client: 2
current: 6619110
highest: 0
starting: 0
client: 1
current: 6619110
highest: 0
starting: 0
client: 2
...
client: 1
current: 6619110
highest: 6666234
starting: 6619110
INFO [04-06|14:21:27.332] Imported new block headers count=192 elapsed=115.877ms number=6,619,302 hash=de8596..cfb8c4 age=1w1d3h
INFO [04-06|14:21:27.533] Imported new block headers count=192 elapsed=32.827ms number=6,619,494 hash=a119dc..f08c74 age=1w1d2h
INFO [04-06|14:21:27.818] Imported new block headers count=192 elapsed=33.990ms number=6,619,686 hash=9c59c7..50f588 age=1w1d2h
client: 2
current: 6619110
highest: 0
starting: 0
...
client: 1
current: 6666242
highest: 6666242
starting: 6619110
...
client: 2
current: 6619110
highest: 6666272
starting: 6619110
...
client: 2
current: 6666279
highest: 6666279
starting: 6619110
There was a problem hiding this comment.
So, I think you are right. My way was just working because prog.CurrentBlock == prog.HighestBlock would still be not syncing... Otherwise the header check was stopping it?
| // wait and lock times are multiplied by testnetSecPerBlock. Tests may | ||
| // need to be run with a high --timeout=2h for the initial sync. |
There was a problem hiding this comment.
So after the first time you run this harness, does the node data persist (and not get wiped) so another harness run can use it instead of syncing from scratch?
There was a problem hiding this comment.
Also, it would be swell if we could specify testnet bootnodes in case you have a local geth running to save internet bandwidth use.
There was a problem hiding this comment.
So after the first time you run this harness, does the node data persist (and not get wiped) so another harness run can use it instead of syncing from scratch?
This does not use the harness at all. So yes, the folder where these nodes live persists until you delete it manually. Also, they have testnet funds in them now, half of what you sent me.
Also, it would be swell if we could specify testnet bootnodes
Will add this ability.
There was a problem hiding this comment.
This does not use the harness at all.
Right right, there's only the light nodes for the dex wallets, and those just use testnet via internet or the specified bootnode. Got it.
|
Running the syncing in two goroutines doesn't seem to matter as they do not sync at the same time for me. |
JoeGruffins
force-pushed
the
ethtestontestnet
branch
from
075c2d5
to
cce3ec0
Compare
| if err != nil { | ||
| return 1, err | ||
| } | ||
| err = setupWallet(participantWalletDir, participantWalletSeed, "localhost:30356", dex.Simnet) |
There was a problem hiding this comment.
What is confusing me is that we've got a shared seed... on testnet? It's gonna get jacked.
If this works for you, OK, we can merge the PR, but I don't understand how the seed cannot be local and private.
There was a problem hiding this comment.
I think it's fine so everyone can test. Would need to be a fairly technical person to jack the testnet coins, and then I don't think they are worth real money... so no incentive?
I can drain the funds and do something else if you like. Some of the tests don't need funds and would work anyway. For instance TestBasicRetrieval
There was a problem hiding this comment.
I don't care about the funds that are in there presently. But also what happens when two devs are using the same wallet at the same time? I have a hard time seeing this work well on a public testnet.
I'm trying to understand if there's a way to have this work on testnet but with a seed that can be specified locally, either by editing the code or by an -ldflags switch.
There was a problem hiding this comment.
What if if the testnet seed/key could be specified locally, and there were a special test like TestCreateTestnetWallet that you could run to make the wallet and native light node, and it would spit out an address at the end of the test, then you could fund it, wait for confs, then run the other testnet tests now that there is funding there.
There was a problem hiding this comment.
Alternative the testnet tests could look for some text file in an expected location for the seed?
There was a problem hiding this comment.
But also what happens when two devs are using the same wallet at the same time?
Yeah, that would cause unexpected results.
There was a problem hiding this comment.
Ok. Will add test that tests for funds and fails if there are not enough to run tests that need funds. It will also spit out the addresses that need funds.
And can the testnet seed just be a separate const? I already have the testnet switch as a constant... and the peer. So, following that pattern.
There was a problem hiding this comment.
JoeGruffins
force-pushed
the
ethtestontestnet
branch
from
cce3ec0
to
24c4d5b
Compare
|
Just rebased. And squashed. |
JoeGruffins
force-pushed
the
ethtestontestnet
branch
2 times, most recently
from
e73f115
to
271de4a
Compare
|
Oh, conflicts, will resolve. |
If testnet is set to true, create a new folder and wallets in the user's home directory and change set up to create testnet nodes. Testing is done pointing at testnet and the testnet contract. Tests currently often fail because a transaction randomly cannot be found to retrieve its receipt. This should not be an issue for swaps as we only check for txn in tests to verify gas used. It may, however, have implications for checking RegFeeConfirmations, as that similarly must find a tx.
JoeGruffins
force-pushed
the
ethtestontestnet
branch
from
271de4a
to
f19e754
Compare
Even waiting and trying many times to find some tx, they just seem to be randomly not existing. Maybe light nodes just can't be expected to find txn? See ethereum/go-ethereum#24101
We should verify if this is normal or maybe we are doing something wrong. I'm guessing it's a problem on testnet and not so much on simnet because of the volume of transactions. Purely a guess at this point though.
Good news is that there doesn't seem to be any unexpected problems with the testnet contract.