[ANN] cbitcoin 2.0 - A Bitcoin Library in C

[GMD1987]

MatthewLM,
On Linux crunchbang 3.2.0-0.bpo.4-686-pae #1 SMP Debian 3.2.35-2~bpo60+1 i686 GNU/Linux, cloned and ran ./configure && make test.  Error on openssl libs, apt-get installed libssl-dev zlib1g-dev, tried again but got the following for dependencies/random/CBRand.c
dependencies/random/CBRand.c: In function ‘CBNewSecureRandomGenerator’:
dependencies/random/CBRand.c:33: error: cast from pointer to integer of different size
dependencies/random/CBRand.c: In function ‘CBSecureRandomSeed’:
dependencies/random/CBRand.c:38: error: cast to pointer from integer of different size
dependencies/random/CBRand.c: In function ‘CBRandomSeed’:
dependencies/random/CBRand.c:41: error: cast to pointer from integer of different size
dependencies/random/CBRand.c:42: error: cast to pointer from integer of different size
dependencies/random/CBRand.c: In function ‘CBSecureRandomInteger’:
dependencies/random/CBRand.c:45: error: cast to pointer from integer of different size
dependencies/random/CBRand.c:45: error: cast to pointer from integer of different size
dependencies/random/CBRand.c:47: error: cast to pointer from integer of different size
dependencies/random/CBRand.c: In function ‘CBFreeSecureRandomGenerator’:
dependencies/random/CBRand.c:51: error: cast to pointer from integer of different size

Looking at the code, uint64_t was used (and designed for this sort of error, no?)
bool CBNewSecureRandomGenerator(uint64_t * gen){
        *gen = (uint64_t)malloc(32);
        return *gen;
}

Do you know what's going on here?  I'm pretty sure the cast it's referring to is in the assignment.  I think this was initially a warning converted to an error and just turning off pedantic would solve it but would be antithetical to a make test 

[MatthewLM]

Hi GMD1987. Thanks for trying it out. I make all warnings into errors to help spot problems (warnings easily go unnoticed otherwise). I didn't get this since I'm using 64-bit. I'm assuming the correct way to suppress this would be to use "-Wno-pointer-to-int-cast" so I'll add this to the makefile now.

testCBNetworkCommunicator is sometimes problematic. I'm going to go through all the tests and run them with valgrind to detect any problems I've missed so far.

[notme]

Hi GMD1987. Thanks for trying it out. I make all warnings into errors to help spot problems (warnings easily go unnoticed otherwise). I didn't get this since I'm using 64-bit. I'm assuming the correct way to suppress this would be to use "-Wno-pointer-to-int-cast" so I'll add this to the makefile now.

testCBNetworkCommunicator is sometimes problematic. I'm going to go through all the tests and run them with valgrind to detect any problems I've missed so far.

I don't think that code is doing what you think it is doing unless you are trying to do something very tricky.  If that is the case, you should document it with a few comments.

Do you really intend to convert the newly allocated pointer to an integer and then return it as a boolean?

[davout]

@MatthewLM
I think the work you're doing is awesome and I thank you for putting your skill and intelligence at work for the common good.

[MatthewLM]

Do you really intend to convert the newly allocated pointer to an integer and then return it as a boolean?

Yes, believe it or not. I was in two minds as to whether or not this is a good idea or not, but for the dependencies I've used uint64_t instead of void * so that it is friendly with integer identifiers (These function prototypes are designed so that they can be implemented in different ways). But now I'm closer to using void * instead. What do others think?

@MatthewLM
I think the work you're doing is awesome and I thank you for putting your skill and intelligence at work for the common good.

Thank you.

[MatthewLM]

These valgrind errors I'm getting are driving me mad:

Code:

==1886== Syscall param socketcall.sendto(msg) points to uninitialised byte(s)
==1886==    at 0x4E412CC: send (send.c:33)
==1886==    by 0x54881B0: CBSocketSend (CBLibEventSockets.c:287)
==1886==    by 0x506953D: CBNetworkCommunicatorOnCanSend (CBNetworkCommunicator.c:459)
==1886==    by 0x5487F43: CBCanSend (CBLibEventSockets.c:235)
==1886==    by 0x663454B: event_base_loop (event.c:1346)
==1886==    by 0x5487B29: CBStartEventLoop (CBLibEventSockets.c:154)
==1886==    by 0x4E39E99: start_thread (pthread_create.c:308)
==1886==  Address 0x6f69774 is 20 bytes inside a block of size 24 alloc'd
==1886==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1886==    by 0x506903B: CBNetworkCommunicatorOnCanSend (CBNetworkCommunicator.c:394)
==1886==    by 0x5487F43: CBCanSend (CBLibEventSockets.c:235)
==1886==    by 0x663454B: event_base_loop (event.c:1346)
==1886==    by 0x5487B29: CBStartEventLoop (CBLibEventSockets.c:154)
==1886==    by 0x4E39E99: start_thread (pthread_create.c:308)
==1886==  Uninitialised value was created by a heap allocation
==1886==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1886==    by 0x5067E55: CBNewNetworkCommunicator (CBNetworkCommunicator.c:30)
==1886==    by 0x4022C2: main (testCBNetworkCommunicator.c:286)
==1886== 
==1886== Syscall param socketcall.sendto(msg) points to uninitialised byte(s)
==1886==    at 0x4E412CC: send (send.c:33)
==1886==    by 0x54881B0: CBSocketSend (CBLibEventSockets.c:287)
==1886==    by 0x5069684: CBNetworkCommunicatorOnCanSend (CBNetworkCommunicator.c:480)
==1886==    by 0x5487F43: CBCanSend (CBLibEventSockets.c:235)
==1886==    by 0x663454B: event_base_loop (event.c:1346)
==1886==    by 0x5487B29: CBStartEventLoop (CBLibEventSockets.c:154)
==1886==    by 0x4E39E99: start_thread (pthread_create.c:308)
==1886==  Address 0x6f6918a is 106 bytes inside a block of size 110 alloc'd
==1886==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1886==    by 0x5061449: CBInitByteArrayOfSize (CBByteArray.c:133)
==1886==    by 0x5061088: CBNewByteArrayOfSize (CBByteArray.c:48)
==1886==    by 0x506C433: CBNetworkCommunicatorSendMessage (CBNetworkCommunicator.c:1307)
==1886==    by 0x506882C: CBNetworkCommunicatorDidConnect (CBNetworkCommunicator.c:217)
==1886==    by 0x5487E01: CBDidConnect (CBLibEventSockets.c:210)
==1886==    by 0x663454B: event_base_loop (event.c:1346)
==1886==    by 0x5487B29: CBStartEventLoop (CBLibEventSockets.c:154)
==1886==    by 0x4E39E99: start_thread (pthread_create.c:308)
==1886==  Uninitialised value was created by a heap allocation
==1886==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==1886==    by 0x5067E55: CBNewNetworkCommunicator (CBNetworkCommunicator.c:30)
==1886==    by 0x4022C2: main (testCBNetworkCommunicator.c:286)

According to get_vbits, the checksum is not defined at https://github.com/MatthewLM/cbitcoin/blob/master/src/CBNetworkCommunicator.c#L457

[Mike Hearn]

Yes, believe it or not. I was in two minds as to whether or not this is a good idea or not, but for the dependencies I've used uint64_t instead of void * so that it is friendly with integer identifiers (These function prototypes are designed so that they can be implemented in different ways). But now I'm closer to using void * instead. What do others think?

Are you sure you want to try and write OOM-safe software? I know from experience that trying to handle NULL returns from malloc is insanely difficult to do correctly, almost no software these days even tries. Aborting the program if malloc returns NULL is invariably the right decision.

Also casting pointers to integers like that is likely to cause issues, which is why the compiler is warning you about it. Don't disable compiler warnings! They are there to help you. The entire Google codebase compiles with -Werror even though it's hundreds of millions of lines, because otherwise it's too easy to ignore them and have bugs sneak past. I'm not sure what you mean by "friendly with integer identifiers" but whatever the goal is here, it's not worth it, especially not for software where correctness is paramount.

The valgrind stack trace doesn't match the code on github, by the way, so if you're asking for help there you'd need to post one with fresh line numbers. The code in that part of the file is very hard to read. I strongly recommend you use structs to avoid code like this:

         // Checksum
         memcpy(peer->sendingHeader + 20, toSend->checksum, 4);

+ 20 here is a magic number, it's not needed because the message headers have a predictable layout anyway so you could just cast the entire header to a struct and then use real identifiers.

Also, try to avoid redefining the language. Here:

      // Need to send the header
      if (NOT peer->messageSent) {
         // Create header
         peer->sendingHeader = malloc(24);

peer->messageSent looks like it's being treated as a boolean (i assume that NOT is defined to be !) but then later on it's treated as if it was an int. If it's a number it's better to write out the integer comparison explicitly.

[MatthewLM]

Hello Mike and thank you for taking a look.

Are you sure you want to try and write OOM-safe software? I know from experience that trying to handle NULL returns from malloc is insanely difficult to do correctly, almost no software these days even tries. Aborting the program if malloc returns NULL is invariably the right decision.

I don't know about other systems but Linux has an OOM killer that makes checking malloc redundant anyway. However, I'm adding checks for the sake of better software regardless. It's not hard to add these checks. Testing them is more difficult. Handling OOM conditions is more important when you are dealing with embedded systems and various low memory environments, so if ever the ode was to be used for these applications it would become more useful.

Also casting pointers to integers like that is likely to cause issues, which is why the compiler is warning you about it.

The compiler is not warning about casting pointers to integers, it's warning about casting a pointer to an integer of a different size. This is not a problem in this case but it doesn't matter since I'm changing my code to use void * instead.

Don't disable compiler warnings! They are there to help you.

Warnings are not necessarily problems. I could perhaps use GCC diagnostic ignored on certain files to avoid making errors ignored for all files.

The valgrind stack trace doesn't match the code on github, by the way, so if you're asking for help there you'd need to post one with fresh line numbers.

OK I'll fix that.

The code in that part of the file is very hard to read. I strongly recommend you use structs to avoid code like this:

         // Checksum
         memcpy(peer->sendingHeader + 20, toSend->checksum, 4);

+ 20 here is a magic number, it's not needed because the message headers have a predictable layout anyway so you could just cast the entire header to a struct and then use real identifiers.

I'll add an enum for the offsets, but I can't use a struct since this data is to be passed through the network.

Also, try to avoid redefining the language. Here:

      // Need to send the header
      if (NOT peer->messageSent) {
         // Create header
         peer->sendingHeader = malloc(24);

peer->messageSent looks like it's being treated as a boolean (i assume that NOT is defined to be !) but then later on it's treated as if it was an int. If it's a number it's better to write out the integer comparison explicitly.

I have the habit of using !/NOT (I've used NOT since I always overlook !) to mean == 0. I can see why that may be misleading. It will take a while to convert all instances of that, but I'll add an issue on github to record that.

[Mike Hearn]

I don't know about other systems but Linux has an OOM killer that makes checking malloc redundant anyway. However, I'm adding checks for the sake of better software regardless. It's not hard to add these checks. Testing them is more difficult. Handling OOM conditions is more important when you are dealing with embedded systems and various low memory environments, so if ever the ode was to be used for these applications it would become more useful.

In practice unless you write/obtain tools that allow systematic exploration of the control flow graph with malloc returns being mocked out, you will never write correct OOM handling software. As you point out, returning NULL when malloc fails is easy, doing something sensible with it is not. In practice you can't do anything sensible with it, so it's not worth trying, it just slows down and clutters up the code.

I'll add an enum for the offsets, but I can't use a struct since this data is to be passed through the network.

Learn about __attribute__((packed)) and bit fields. You can use a struct to overlay any arbitrary piece of memory.

[MatthewLM]

Checking for malloc returning NULL is hardly going to slow down the code. Though saying it clutters up the code is a fair point.

Learn about __attribute__((packed)) and bit fields. You can use a struct to overlay any arbitrary piece of memory.

I do not want to do this as it is bad for portability.

I'm not going to use void * after-all as I just remembered the network code uses integers returned by socket(). Best keep it the way it was before. uint64_t can take any pointer or integer, so it's safest to just use that.

Edit: The valgrind errors do correspond with the line numbers for me.

[jgarzik]

I'm not going to use void * after-all as I just remembered the network code uses integers returned by socket(). Best keep it the way it was before. uint64_t can take any pointer or integer, so it's safest to just use that.

You want to use, not work around, the C type system.  Use the type that most accurately reflects the contents.  Google for alias analysis, as just one of many examples why you should not "hide" things from the compiler.

[MatthewLM]

But the contents are unknown. Either an integer or a pointer could be used depending on the implementation. And any optimisations would be useless in this case.

[jgarzik]

But the contents are unknown. Either an integer or a pointer could be used depending on the implementation. And any optimisations would be useless in this case.

No, you're doing something wrong if you're stuffing/casting pointers into integers like that.  File descriptors and handles returned by the OS are different in every way from a pointer.

[MatthewLM]

But the contents are unknown. Either an integer or a pointer could be used depending on the implementation. And any optimisations would be useless in this case.

No, you're doing something wrong if you're stuffing/casting pointers into integers like that.  File descriptors and handles returned by the OS are different in every way from a pointer.

Yes they are different than pointers, hence why I'm using a uint64_t that can hold all integers and all pointers. These functions that have uint64_t arguments are declared in the core library only as weakly linked prototypes. The functions are implemented outside this core library. These implementations can use integers or pointers. I could make assumptions such as that these implementations will only want to use integers for sockets, pointers for random number generators etc. but I feel better making so that people can implement them with integers or pointers as they choose up to 64 bits.

From what I know this causes no problems. Any losses in optimisation would be too small to worry about.

The only thing the core library that holds the integer representation does, is store that integer and pass it into functions that are implemented outside the library. These implementations will cast the integer into the appropriate type for that implementation.

[Mike Hearn]

I do not want to do this as it is bad for portability.

Every compiler supports struct packing. See here:

http://stackoverflow.com/questions/1537964/visual-c-equivalent-of-gccs-attribute-packed/3312896#3312896

[MatthewLM]

OK Mike. I see four problems with this:

1. It's more confusing and counter-intuitive in my opinion, though different people may think differently of-course.
2. The way I do it works and is simple enough.
3. What about endianness?
4. What about compilers for embedded systems, which cbitcoin may (may being an important word) be suitable for in the future?

[K1773R]

OK Mike. I see four problems with this:

1. It's more confusing and counter-intuitive in my opinion, though different people may think differently of-course.
2. The way I do it works and is simple enough.
3. What about endianness?
4. What about compilers for embedded systems, which cbitcoin may (may being an important word) be suitable for in the future?

4. show me a emdedded systems architecture where gcc cant compile for it?

[MatthewLM]

Are you saying that practically all architectures have compilers that support the packed attribute?

There are the other three reasons to keep it the way it is.

[EndTheFed321]

Good luck with this one hope you reach your goal 

[Mike Hearn]

Are you saying that practically all architectures have compilers that support the packed attribute?

There are the other three reasons to keep it the way it is.

Yes, that is what he is saying.

Bitcoin is inherently a little endian protocol because Satoshi uses memcpy in a few places in the serialization code, and because it runs on x86 chips which are LE only. Your code is also endian-specific for the same reason.