proposer: wait for a hash to be in the active-leaves set

[ordian]

Fixes #1483.

This solution feels hacky to me, so I'd like to hear your thoughts on this.

I don't know why active_leaves was defined as a HashMap<Hash, BlockNumber> and if there are any downsides to making it a HashMap.

[coriolinus]

This isn't yet ready, but it seems a plausible direction to take things. My biggest concern right now is ensuring that activaion_external_listeners never experiences unbounded growth, which it currently does.

[ordian]

the only downside to this is if a lot of hashes are interesting so this is slow, or if block importing stopped for some reason, but both scenarios seem unlikely

[coriolinus]

Agree.

[coriolinus]

@ordian I no longer need to request changes, but I note that this hasn't yet been marked as ready for review. Would you like me to approve it now, or did you have further work in mind before calling this complete?

[ordian]

@ordian I no longer need to request changes, but I note that this hasn't yet been marked as ready for review. Would you like me to approve it now, or did you have further work in mind before calling this complete?

I was trying to add a test before making the PR as "ready for review", but it seems tricky w/o the ability to inspect overseer's internal state, so I'll omit it unless you have suggestions.

[coriolinus]

Tests are always nice. One technique I've seen used for unit testing purposes is to write a method like this:

#[cfg(test)]
pub fn activation_external_listeners(&self) -> HashMap<_, _> { ... }

which exposes the internal state, but only when compiled in test mode.

I'll leave it to you whether to write some tests using a feature like that; I am willing to approve the PR in its current state.

[ordian]

Tests are always nice. One technique I've seen used for unit testing purposes is to write a method like this:

#[cfg(test)]
pub fn activation_external_listeners(&self) -> HashMap<_, _> { ... }

which exposes the internal state, but only when compiled in test mode.

I'll leave it to you whether to write some tests using a feature like that; I am willing to approve the PR in its current state.

the problem is that overseer.run consumes overseer, other workarounds are unfortunate, so I'll leave it as is for now

let me know what you think about adding debug_assertions : #1616 (comment)

[ordian]

Thanks for the review and quick replies!

[rphmeier]

Seems like there would be a memory leak if i requested to wait for a hash of a block that will never be in the active-leaves set.

[ordian]

Seems like there would be a memory leak if i requested to wait for a hash of a block that will never be in the active-leaves set.

this is handled by assuming the caller handles timeouts and will drop the receiver (this is needed also in case the a hash is deactivated before subscription) - this is documented, so we will clean it up on block_imported

[ordian]

bot merge

[ghost]

Trying merge.