Bigendian fixes and CI test #627
Conversation
they are a noop on little-endian and the following {to/from}_array_le are sufficient to deal with big-endian
| addr[4].to_be(), addr[5].to_be(), addr[6].to_be(), addr[7].to_be()] | ||
| let mut result = addr.clone(); | ||
| for i in 0..8 { | ||
| result[i] = result[i].swap_bytes(); |
There was a problem hiding this comment.
It's not obvious to me why it should be swap_bytes and not to_be, could you expand on that?
There was a problem hiding this comment.
To be completely honest I've seen successful test on little-endian failing with swapped bytes on big-endian arch, so I checked if failing tests had test vector shared with bitcoin core, and I've see it's the case for [de]serialize_addrv2_test, so I swapped bytes also on big-endian (to_be() is a noop on big-endian) to make tests happy ( I need to rename the fn to something likeaddr_to_swapped...)
There was a problem hiding this comment.
@elichai because consensus_encode always encodes in LE, and we want to instead always encode in BE.
I agree it's confusing but this is probably the most sensible thing to do..
There was a problem hiding this comment.
Maybe we should instead change the address type to something other than 8 u16s? u16s are pretty strange for address representation IMO.
There was a problem hiding this comment.
I think it makes sense to change the address type, but should be done in another PR since this is a bugfix
| @@ -81,7 +84,7 @@ impl Encodable for Address { | |||
| ) -> Result<usize, io::Error> { | |||
| let len = self.services.consensus_encode(&mut s)? | |||
| + addr_to_be(self.address).consensus_encode(&mut s)? | |||
| + self.port.to_be().consensus_encode(s)?; | |||
| + self.port.swap_bytes().consensus_encode(s)?; | |||
There was a problem hiding this comment.
Can you add a comment as to why swap_bytes here and below, maybe pasting andrew's review comment?
There was a problem hiding this comment.
Added comments, also suggesting to switch to other code when MSRV allow that
801c378 disable illumos and netbsd (Riccardo Casatta) a426456 [CI] add cache (Riccardo Casatta) f1bdee2 add cross testing on rust tier 1 and tier 2 with host tools (Riccardo Casatta) Pull request description: After working on rust-bitcoin/rust-bitcoin#627 I thought it may be simple and useful to use [cross](https://github.com/rust-embedded/cross) environment to test across different architectures and started here. So I took all rust tier1 and tier2 with Host tools archictectures and added to the test matrix, run here https://github.com/RCasatta/rust-secp256k1/actions/runs/985791240. Errors on darwin are due to the environment because it works fine on physical machine, however errors on illumos and netbsd maybe are useful to know? ACKs for top commit: apoelstra: utACK 801c378 Tree-SHA512: 43c7220e41856344d4a932426d8acb8e9f004fcf33acfbde4b26f3a6074b0ce3e766d99afaca6c18381a4438775fa693b06c70d3204d83ff5a97fcbebe126056
fixes #624
Adds a CI job using cross https://github.com/rust-embedded/cross to run tests on bigendian arch
At the moment it is using
s390x-unknown-linux-gnuwhich is the arch used by @jlatone in the referenced issue but we can easily extend via matrix if we want to test other arch.