-
Notifications
You must be signed in to change notification settings - Fork 122
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.
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
PoolSv2
integration tests
#1066
base: main
Are you sure you want to change the base?
Conversation
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
0dde2c8
to
9dc8a6c
Compare
8ace1ac
to
e90b991
Compare
c0dbde4
to
fbb8464
Compare
56c26c0
to
6cfc1c7
Compare
a3f154b
to
0be5cc6
Compare
ebdc14c
to
69c5dcf
Compare
Handle errors in `start` function for better user experience and to be able to catch errors in test environment, for example without introducing error handling, we do not get a proper response if the provided `coinbase_output` in the config is valid.
Adds a new property `state` to the pool struct. The main goal is providing abilities to mutate the state internally, i.e. only by the role itself, and abilities to view the state by the role runner.
69c5dcf
to
3a80407
Compare
@Shourya742 @plebhash Thanks for the ongoing review. This is ready for another round when you have a minute |
Bencher Report
Click to view all benchmark results
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK. Just few nits and questions.
listening_address: Option<std::net::SocketAddr>, | ||
coinbase_outputs: Option<Vec<pool_sv2::mining_pool::CoinbaseOutput>>, | ||
template_provider_address: Option<std::net::SocketAddr>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
listening_address: Option<std::net::SocketAddr>, | |
coinbase_outputs: Option<Vec<pool_sv2::mining_pool::CoinbaseOutput>>, | |
template_provider_address: Option<std::net::SocketAddr>, | |
listening_address: Option<SocketAddr>, | |
coinbase_outputs: Option<Vec<pool_sv2::mining_pool::CoinbaseOutput>>, | |
template_provider_address: Option<SocketAddr>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed
pub async fn start_template_provider_and_pool() -> Result<(PoolSv2, u16, TemplateProvider, u16), ()> | ||
{ | ||
let (template_provider, template_provider_port) = start_template_provider().await; | ||
let template_provider_address = | ||
SocketAddr::from_str(&format!("127.0.0.1:{}", template_provider_port)).unwrap(); | ||
let test_pool = TestPoolSv2::new(None, None, Some(template_provider_address)); | ||
let pool = test_pool.pool.clone(); | ||
let state = pool.state().await.safe_lock(|s| s.clone()).unwrap(); | ||
assert_eq!(state, pool_sv2::PoolState::Initial); | ||
let _pool = pool.clone(); | ||
tokio::task::spawn(async move { | ||
assert!(_pool.start().await.is_ok()); | ||
}); | ||
// Wait for the pool to start. | ||
tokio::time::sleep(Duration::from_secs(1)).await; | ||
let pool_listening_address = | ||
SocketAddr::from_str(&format!("127.0.0.1:{}", test_pool.port)).unwrap(); | ||
loop { | ||
if is_port_open(pool_listening_address) { | ||
break; | ||
} | ||
} | ||
let state = pool.state().await.safe_lock(|s| s.clone()).unwrap(); | ||
assert_eq!(state, pool_sv2::PoolState::Running); | ||
template_provider.stop(); | ||
Ok(( | ||
pool, | ||
test_pool.port, | ||
template_provider, | ||
template_provider_port, | ||
)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we having a method to spun up both template provider and pool together in common module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main motivation here is to provide users(devs) with a single function to start both the TP and a Pool so they don't need to rewrite all of this code again and again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while this seems convenient at a first glance, I wonder how it will scale?
if we think that there's 7 different roles, and there could be scenarios where we want more than one instance (e.g.: 2 proxies connected to the same pool), the combinatorial possibilities can explode very quickly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should more useful in testing scenarios that involve the pool
, i.e., the MG tests that are prefixed with pool_
. All the pool
tests need to start a TP before starting the pool
so I wanted to avoid duplicating the code.
An example can be seen here: https://github.com/stratum-mining/stratum/pull/1164/files#diff-2a0b80af72100f3ef6475d64910e57bfa2194a668b3a7692391a53f964340fb4R68
all the code from L38 to L68 can be removed and the test will be much nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following this logic, we would need to implement similar methods for JDC and TP setup as well. This approach becomes problematic when setting up a full Job Declarator configuration, as both methods would be ineffective due to the need for TP instances to use different ports. For each test, we may end up with some duplicated setup code, which could serve as a framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like this?
pub async fn start_template_provider_and_pool() -> Result<(PoolSv2, u16, TemplateProvider, u16), ()> | |
{ | |
let (template_provider, template_provider_port) = start_template_provider().await; | |
let template_provider_address = | |
SocketAddr::from_str(&format!("127.0.0.1:{}", template_provider_port)).unwrap(); | |
let test_pool = TestPoolSv2::new(None, None, Some(template_provider_address)); | |
let pool = test_pool.pool.clone(); | |
let state = pool.state().await.safe_lock(|s| s.clone()).unwrap(); | |
assert_eq!(state, pool_sv2::PoolState::Initial); | |
let _pool = pool.clone(); | |
tokio::task::spawn(async move { | |
assert!(_pool.start().await.is_ok()); | |
}); | |
// Wait for the pool to start. | |
tokio::time::sleep(Duration::from_secs(1)).await; | |
let pool_listening_address = | |
SocketAddr::from_str(&format!("127.0.0.1:{}", test_pool.port)).unwrap(); | |
loop { | |
if is_port_open(pool_listening_address) { | |
break; | |
} | |
} | |
let state = pool.state().await.safe_lock(|s| s.clone()).unwrap(); | |
assert_eq!(state, pool_sv2::PoolState::Running); | |
template_provider.stop(); | |
Ok(( | |
pool, | |
test_pool.port, | |
template_provider, | |
template_provider_port, | |
)) | |
pub async fn start_template_provider_and_pool(tp_port: Option<..>) -> Result<(PoolSv2, u16, TemplateProvider, u16), ()> | |
{ | |
let (template_provider, template_provider_port) = start_template_provider(tp_port).await; | |
let template_provider_address = | |
SocketAddr::from_str(&format!("127.0.0.1:{}", template_provider_port)).unwrap(); | |
let test_pool = TestPoolSv2::new(None, None, Some(template_provider_address)); | |
let pool = test_pool.pool.clone(); | |
let state = pool.state().await.safe_lock(|s| s.clone()).unwrap(); | |
assert_eq!(state, pool_sv2::PoolState::Initial); | |
let _pool = pool.clone(); | |
tokio::task::spawn(async move { | |
assert!(_pool.start().await.is_ok()); | |
}); | |
// Wait for the pool to start. | |
tokio::time::sleep(Duration::from_secs(1)).await; | |
let pool_listening_address = | |
SocketAddr::from_str(&format!("127.0.0.1:{}", test_pool.port)).unwrap(); | |
loop { | |
if is_port_open(pool_listening_address) { | |
break; | |
} | |
} | |
let state = pool.state().await.safe_lock(|s| s.clone()).unwrap(); | |
assert_eq!(state, pool_sv2::PoolState::Running); | |
template_provider.stop(); | |
Ok(( | |
pool, | |
test_pool.port, | |
template_provider, | |
template_provider_port, | |
)) |
And you are right, at the beginning there is a bit of overhead needing to add the different start
functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still not convinced about this. The example I provided was just one specific case. If, for instance, we want to change other configuration properties of any of the involved entities, it would require further modifications to the function signature. All functions should adhere to the Open/Closed Principle: they should be open for extension but closed to modification.
3a80407
to
4542302
Compare
Bencher Report
Click to view all benchmark results
|
A utility struct that wraps the original `PoolSv2` and provide some utility to start the pool role in testing env.
4542302
to
5ccc297
Compare
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
I have been working a bit on this PR in order to improve our testing abilities. I wanted to mainly be able to aggregate each exchanged message between two roles. In the test you can see now that there is an assertion on the messages https://github.com/stratum-mining/stratum/pull/1066/files#diff-2a0b80af72100f3ef6475d64910e57bfa2194a668b3a7692391a53f964340fb4R39 The PR is currently not passing CI because its linking to a local version of |
Damn! This is amazing! Demand-easy-sv2 to the rescue—I knew it could help with the message syncing.. |
Blocked by #1155 and #1156
for more context: