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.

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

Bump cardano-node to 1.32.1 #3050

Merged
merged 8 commits into from
Dec 15, 2021
Merged

Conversation

rvl
Copy link
Contributor

@rvl rvl commented Dec 9, 2021

Updates to cardano-node 1.32.1

Release notes for 1.31.0.

  • Updates cardano-node version in stack.yaml.
  • Updates cardano-node version in cabal.project.
  • Fix new build errors.
  • Fix any test failures.
  • Update readme.

Comments

See also: #3023 #3038

Issue Number

ADP-1266

@rvl rvl self-assigned this Dec 9, 2021
@rvl rvl force-pushed the rvl/adp-1266/cardano-node-1.32.1 branch from a560d27 to e4ee513 Compare December 9, 2021 17:08
@rvl
Copy link
Contributor Author

rvl commented Dec 9, 2021

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 9, 2021

👎 Rejected by too few approved reviews

@rvl
Copy link
Contributor Author

rvl commented Dec 9, 2021

bors try

iohk-bors bot added a commit that referenced this pull request Dec 9, 2021
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 9, 2021

try

Build failed:

@rvl rvl force-pushed the rvl/adp-1266/cardano-node-1.32.1 branch from e4ee513 to 308a7fe Compare December 14, 2021 06:35
@rvl
Copy link
Contributor Author

rvl commented Dec 14, 2021

bors try

iohk-bors bot added a commit that referenced this pull request Dec 14, 2021
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 14, 2021

try

Build succeeded:

Copy link
Member

@Anviking Anviking left a comment

Choose a reason for hiding this comment

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

Great! lgtm modulo three comments which I think should be addressed.

(Large mem) <- arbitrary
pure $ ExecutionUnits steps mem
(Large (steps :: Int)) <- arbitrary
(Large (mem :: Int)) <- arbitrary
Copy link
Member

Choose a reason for hiding this comment

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

Using Word64 might be the safer choice to preserve the previous behaviour. (ExecuionUnits had Word64 fields on 1.30.1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - fixed.

Just supported -> oneof
[ pure TxOutDatumHashNone
[ pure TxOutDatumNone
, TxOutDatumHash supported <$> genHashScriptData
Copy link
Member

@Anviking Anviking Dec 14, 2021

Choose a reason for hiding this comment

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

Interesting, it seems there's both TxOutDatum' and TxOutDatumHash now.

Could we rename genTxOutDatumHash to genTxOutDatum and also generate TxOutDatum'?

Or at the least add a note about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed genTxOutDatumHash to genTxOutDatum, but where is this TxOutDatum' ?

Copy link
Member

Choose a reason for hiding this comment

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

{ executionSteps = steps
, executionMemory = mem
{ executionSteps = fromIntegral steps
, executionMemory = fromIntegral mem
Copy link
Member

@Anviking Anviking Dec 14, 2021

Choose a reason for hiding this comment

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

This looks like an unsafe Natural to Word64 conversion.

I believe this is something we really don't want to overflow (even if it's impossible or has no effect at the moment).

Can we change W.ExecutionUnits to also use Natural instead of Word64?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, definitely.

@Anviking
Copy link
Member

Also don't forget to update the readme compatibility table!

@kk-hainq kk-hainq mentioned this pull request Dec 14, 2021
@Anviking Anviking force-pushed the rvl/adp-1266/cardano-node-1.32.1 branch from ee17530 to 93b934b Compare December 14, 2021 22:35
@Anviking
Copy link
Member

I rebased, some build error remains though

@rvl rvl force-pushed the rvl/adp-1266/cardano-node-1.32.1 branch from 93b934b to 5bbf2bd Compare December 15, 2021 13:10
@rvl
Copy link
Contributor Author

rvl commented Dec 15, 2021

Thanks ... but you may have accidentally force pushed over some commits 😁. I have pushed them back now, and will address your review comments shortly.

@rvl rvl force-pushed the rvl/adp-1266/cardano-node-1.32.1 branch from cd8367f to e2fcd53 Compare December 15, 2021 13:23
@Anviking
Copy link
Member

but you may have accidentally force pushed over some commits 😁. I have pushed them back now

🙈 👍

@rvl rvl force-pushed the rvl/adp-1266/cardano-node-1.32.1 branch from adcd22b to 8a133b9 Compare December 15, 2021 13:47
@rvl
Copy link
Contributor Author

rvl commented Dec 15, 2021

bors r+

iohk-bors bot added a commit that referenced this pull request Dec 15, 2021
3050: Bump cardano-node to 1.32.1 r=rvl a=rvl

Updates to [cardano-node 1.32.1](https://github.com/input-output-hk/cardano-node/releases/tag/1.32.1)

Release notes for [1.31.0](https://github.com/input-output-hk/cardano-node/releases/tag/1.31.0).

- [x] Updates cardano-node version in `stack.yaml`.
- [x] Updates cardano-node version in `cabal.project`.
- [x] Fix new build errors.
- [x] Fix any test failures.
- [x] Update readme.

### Comments

- [Hydra jobset](https://hydra.iohk.io/jobset/Cardano/cardano-wallet-pr-3050)

See also: #3023 #3038

### Issue Number

ADP-1266

Co-authored-by: Rodney Lorrimar <[email protected]>
Co-authored-by: Jann Müller <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 15, 2021

Build failed:

src/Test/Integration/Scenario/CLI/Shelley/Transactions.hs:244:59:
--
  | 1) CLI Specifications, SHELLEY_CLI_TRANSACTIONS, TRANS_CREATE_06 - Invalid amount, 1.5
  | uncaught exception: IOException of type ResourceVanished
  | fd:144: hFlush: resource vanished (Broken pipe)

#2500

@Anviking
Copy link
Member

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 15, 2021

Merge conflict.

@rvl rvl force-pushed the rvl/adp-1266/cardano-node-1.32.1 branch from 8a133b9 to 6dacb51 Compare December 15, 2021 16:39
@rvl
Copy link
Contributor Author

rvl commented Dec 15, 2021

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Dec 15, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 7695502 into master Dec 15, 2021
@iohk-bors iohk-bors bot deleted the rvl/adp-1266/cardano-node-1.32.1 branch December 15, 2021 17:57
@rvl rvl mentioned this pull request Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants