@plebhash @jbesraa do you think we need to add a new integration test for this? We are able to assert specific fields value inside specific messages, right?

Also, let me know if you want me to bump roles_logic_sv2 patch version here or not.

Bencher Report

Bencher Report

Bencher Report

Bencher Report

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 19.30%. Comparing base (5fec3f1) to head (fb70488).
Report is 5 commits behind head on main.

@@            Coverage Diff             @@
##             main    #1325      +/-   ##
==========================================
+ Coverage   18.61%   19.30%   +0.68%     
==========================================
  Files         144      164      +20     
  Lines        9718    10853    +1135     
==========================================
+ Hits         1809     2095     +286     
- Misses       7909     8758     +849     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

do you think we need to add a new integration test for this?

IMO more tests are always desirable 🙈

the real question is how complex it is to write a comprehensive test for this bug

if it is too complex and we decide it's not worth spending our time on it now, we could always take note of this on #1077 and come back to it later

We are able to assert specific fields value inside specific messages, right?

yes, see:

or a more concrete example:

Also, let me know if you want me to bump roles_logic_sv2 patch version here or not.

are we doing a patch release for this bugfix?

if yes, then we should bump and release right away

if not, then we should take note of this change and then bump before v1.3.0 (maybe there will be a MAJOR or MINOR bump that would overwrite this)

You should the curtime field, not mintime. For the timewarp scenario that this PR fixes, they're the same. But normally mintime is equal to MTP + 1, which on mainnet is typically an hour in the past. Whereas curtime is the node clock time, adjusted for MTP + 1 and for timewarp where needed.

You should the curtime field, not mintime. For the timewarp scenario that this PR fixes, they're the same. But normally mintime is equal to MTP + 1, which on mainnet is typically an hour in the past. Whereas curtime is the node clock time, adjusted for MTP + 1 and for timewarp where needed.

I'm not following you here, weren't we saying that we should always use the timestamp communicated by the TP?

Maybe I'm just confused by the name. The getblocktemplate RPC has both a curtime and mintime field. But the TP doesn't use that field and just gives you the nTime value in the block. That's not the minimum time, so the variable name mintime is confusing. But it should work correctly.

What I'm using in this PR is the header_timestamp field contained in the SetNewPrevHash message, as specified here. It's correct, right?

Regarding the naming, do you think something could be improved in the specs?

Yes, it's indeed header_timestamp.

The spec is clear:

The nTime field in the block header at which the client should start (usually current time). This is NOT the minimum valid nTime value.

Some part of the SRI codebase is calling it Some(referenced_job.min_ntime). I think that variable should be renamed from min_ntime to either ntime or header_timestamp.

Yes, it's indeed header_timestamp.

The spec is clear:

The nTime field in the block header at which the client should start (usually current time). This is NOT the minimum valid nTime value.

Some part of the SRI codebase is calling it Some(referenced_job.min_ntime). I think that variable should be renamed from min_ntime to either ntime or header_timestamp.

I think that's because on the mining protocol specs is specified as min_ntime: https://github.com/stratum-mining/sv2-spec/blob/main/05-Mining-Protocol.md#5315-newminingjob-server---client

Don't you think we should enhance that part of the specs? I can push changes on this PR, but I think that if we agree on it, we should also clarify specs.

Yes it makes sense to improve the spec there.

Smallest nTime value available for hashing for the new mining job.

There's currently no way to know this, unless we add a new field to the NewTemplate and/or SetNewPrevHash message.

@Sjors took note of this on both:

• specs (min_ntime field name is misleading and description can be improved sv2-spec#119)
• SRI (Refactor: subprotocols crates #1202 (comment))

@plebhash integration test added in e4e5e61.

Wrt version bumping, I would discuss about it during next dev call. So that we can better agree on the need about doing a patch release or not.

Thanks everyone for reviewing it 🙏 , I addressed almost every comment.

The last thing which needs to be done in this PR is to bump the crate version in its Cargo.toml. According to @Shourya742's comment here we should bump MAJOR, but cargo-semver-check is not failing on that.

I'm diving into it, will post updates here asap.

I did some local manual testing for the semver issue:

• make sure cargo semver-checks is 0.37.0 (same as CI)
  • cargo +stable install cargo-semver-checks --version 0.37.0 --locked
• run the tool on roles_logic_sv2
  • cd protocols/v2/roles-logic-sv2; cargo +stable semver-checks --default-features

(the +stable was used because my local cargo is locked on MSRV, while CI runs on stable)

no errors were reported

it's very good that @Shourya742 had the eagle eyes to detect this potential versioning issue, but we should definitely investigate deeper to know if:

• @Shourya742 was mistaken and a PATCH bump is sufficient
• OR we need to fix the CI, as it's not reliable

for future reference:

after some investigation, it was confirmed that the semver issue was a false alarm

although the JobsCreators is a public struct, the added field is private, which therefore does not categorize a MAJOR version bump