#8752 needs some love again

It's been dragging for too long

sech1: I'll review it today

sech1: did you closely review all the test related changes outside trezor tests?

I reviewed all files. If you check comments from my initial review, I was asking some questions about those files. IIRC it was mostly renaming stuff in tests to make them work correctly again.

I'm always a bit careful with merging changes to core_tests

I'm not an expert in core tests though, here's where I asked it: monero-project/monero #8752#pullrequestreview-1492566412

"monero tests: fix tx create from sources, input locking. Originally, creating a synthetic transactions with chaingen could create a transaction with outputs that are still locked in the current block, thus failing chain validation by the daemon. Simple unlock check was added. Some buggy tests were fixed as well as new unlock-checking version of tx

creation rejected those, fixes are simple - mostly using correct block after a rewind to construct a transaction"

"Regarding the blk_5r question - I had to rewind so tests pass after fix of tx lock time."

That sucks. This really should be a separate commit, or it's unreviewable really.

(not to mention unbisectable/unrevertable)

moneromoooo: so one commit for chaingen / core test related changes and the remaining trezor changes in the other commit?

I try to make one commit per fix when I can. It's *usually* not hard to commit a fix separately when you code a larger thing and you uncover some bug while doing so. Not *always* easy, but usually. And since this patch isn't really needed since it's just for trezor, it applies even more here.

Quite often I'll have a string of random fixes or small improvements, then the main patch that adds a feature. It makes it easier to review, bisect, revert piecemeal if needed.

For this particular patch, it might be harder to do if both modify the same code, I've not looked.

But then again, 41 files changed is good incentive not to look...

he will try to split it up

<j​effro256:monero.social> Is it also possible to split the libUSB changes into its own PR ?

the PR already has 160 comments, I feel bad asking for more changes at this point lol

<j​effro256:monero.social> True True

<j​effro256:monero.social> I want to see the core_tests modifications in a different PR, but I can live otherwise

they will be in the same PR but a different commit since otherwise tests wouid fail

<j​effro256:monero.social> Fair enough

Well, a massive patch with unrelated things in it, should have known better. Though I know it's sometimes tempting.

this is just the chaingen patch monero-project/monero #8752/commits/056c9967036175e78b7ac1758d9a2943a9283bfc