Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Client: More UI Improvements / Fix TxPool not being started along FCU #3100

Merged
merged 6 commits into from
Oct 11, 2023
Merged

Client: More UI Improvements / Fix TxPool not being started along FCU #3100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97fdc40
Client: expand super msgs to also take in multiline output
holgerd77 Oct 10, 2023
9bc1d51
Client: adjust CL disconnect thresholds to avoid reconnect messages
holgerd77 Oct 10, 2023
b8a64b7
Client: add note to explain subchains on subchain creation
holgerd77 Oct 10, 2023
cb6fc02
Client: fix synchronized setting on FCU
holgerd77 Oct 10, 2023
eabefcb
Client: make tx pool started a super msg
holgerd77 Oct 10, 2023
8777ade
fix switching client to syncronized state and start txpool
g11tech Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions packages/client/src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,10 +651,18 @@ export class Config {
return key
}

superMsg(msg: string, meta?: any) {
const len = msg.length
superMsg(msgs: string | string[], meta?: any) {
if (typeof msgs === 'string') {
msgs = [msgs]
}
let len = 0
for (const msg of msgs) {
len = msg.length > len ? msg.length : len
}
this.logger.info('-'.repeat(len), meta)
this.logger.info(msg, meta)
for (const msg of msgs) {
this.logger.info(msg, meta)
}
this.logger.info('-'.repeat(len), meta)
}

Expand Down
17 changes: 11 additions & 6 deletions packages/client/src/rpc/modules/engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1225,12 +1225,6 @@ export class Engine {
}
}
this.service.txPool.removeNewBlockTxs(blocks)

const isPrevSynced = this.chain.config.synchronized
this.config.updateSynchronizedState(headBlock.header)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this line be removed? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's was not triggering anything anyhow and is now replaced by a direct setting of synchronized on FCU.

Testing eventual side effects right now by running a full Holesky sync (seems nothing so far).

Point of all this is that the tx pool never started.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should remove this line. The txpool thing is fine. Now each time FCU is called it will set state to synchronized. There is an interval in sync.ts which calls it also, which might override it back to syncing. I think this has undesired side effects..?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess i"m not 100% clear on what this synchronized state represents then. Does it represent the current head that the CL knows of or is it the current canonical head that the client is aware of or yet again, is it the current executed head of the client?

cc @g11tech

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally have messy "synchronized" setting logic, especially in a post-Merge context.

This updateSynchronizedState() method e.g. - post-Mege - didn't trigger at all any more (didn't analyze further), with one (likely beyond other) before-change side effects that the tx pool did not start.

I now updated based on the assumed definition that the chain is synchronized "by definition" after a succesfull FCU including execution.

Side note: that doesn't mean our synchronized-code is completely clean after that. We should take this upon on a separate occasion and generally clean this up and make this more consistent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i will lookinto fhis and make a fix shortly, as discussed with @holgerd77 the new logic might open the txpool without us being in the "synchronized" state

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the fix:
image

if (!isPrevSynced && this.chain.config.synchronized) {
this.service.txPool.checkRunState()
}
} else {
// even if the vmHead is same still validations need to be done regarding the correctness
// of the sequence and canonical-ity
Expand All @@ -1244,6 +1238,17 @@ export class Engine {
}
}

if (
this.config.syncTargetHeight === undefined ||
this.config.syncTargetHeight < headBlock.header.number
) {
this.config.syncTargetHeight = headBlock.header.number
}
this.config.updateSynchronizedState(headBlock.header)
if (this.chain.config.synchronized) {
this.service.txPool.checkRunState()
}

// prepare valid response
let validResponse
// If payloadAttributes is present, start building block and return payloadId
Expand Down
4 changes: 2 additions & 2 deletions packages/client/src/rpc/util/CLConnectionManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,13 @@ export class CLConnectionManager {
private DEFAULT_FORKCHOICE_LOG_INTERVAL = 60000

/** Threshold for a disconnected status decision */
private DISCONNECTED_THRESHOLD = 30000
private DISCONNECTED_THRESHOLD = 45000
/** Wait for a minute to log disconnected again*/
private LOG_DISCONNECTED_EVERY_N_CHECKS = 6
private disconnectedCheckIndex = 0

/** Threshold for an uncertain status decision */
private UNCERTAIN_THRESHOLD = 15000
private UNCERTAIN_THRESHOLD = 30000

/** Track ethereumjs client shutdown status */
private _clientShutdown = false
Expand Down
8 changes: 5 additions & 3 deletions packages/client/src/service/skeleton.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,9 +395,11 @@ export class Skeleton extends MetaDBManager {
next: head.header.parentHash,
}
this.status.progress.subchains.unshift(s)
this.config.superMsg(
`Created new subchain tail=${s.tail} head=${s.head} next=${short(s.next)}`
)
const msgs = [
`Created new subchain tail=${s.tail} head=${s.head} next=${short(s.next)}`,
'Note: Subchain will be backfilled and merged with the canonical chain on success.',
]
this.config.superMsg(msgs)
// Reset the filling of canonical head from tail only on tail reorg and exit any ongoing fill
this.status.canonicalHeadReset = s.tail > BIGINT_0
} else {
Expand Down
2 changes: 1 addition & 1 deletion packages/client/src/service/txpool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ export class TxPool {
this._logInterval = setInterval(this._logPoolStats.bind(this), this.LOG_STATISTICS_INTERVAL)
}
this.running = true
this.config.logger.info('TxPool started.')
this.config.superMsg('TxPool started.')
return true
}

Expand Down
Loading