store: bug fixes

* initialize the pool esp. pool->ver_top
* added the pool to verify
* fixed under allocation due to rounding-up after footprint calc, as discussed in #5911 by validating inputs
* check and clear magic in delete
* added handholding check and docs for calling _clear on an empty store
* Use &fec->key.mr instead implicitly of relying on the layout of the key struct
* Various other nits

Author: two-heart

src/disco/shred/fd_shred_tile.c

@@ -921,7 +921,7 @@ after_frag( fd_shred_ctx_t *    ctx,
 
       long shacq_start, shacq_end, shrel_end;
       FD_STORE_SHACQ_TIMED( ctx->store, shacq_start, shacq_end );
-      fd_store_fec_t * fec = fd_store_insert( ctx->store, (uint)ctx->round_robin_id, (fd_hash_t *)fd_type_pun( &out_merkle_root ) );
+      fd_store_fec_t * fec = fd_store_insert( ctx->store, ctx->round_robin_id, (fd_hash_t *)fd_type_pun( &out_merkle_root ) );
       FD_STORE_SHREL_TIMED( ctx->store, shrel_end );
 
       for( ulong i=0UL; i<set->data_shred_cnt; i++ ) {

src/disco/store/fd_store.c

@@ -38,7 +38,6 @@ fd_store_new( void * shmem, ulong fec_max, ulong part_cnt ) {
   }
 
   fd_memset( shmem, 0, footprint );
-  fec_max = fd_ulong_pow2_up( fec_max ); /* required by map_chain */
 
   /* This seed value is very important. We have fec_max chains in the
      map, which means the size of each partition of buckets should be
@@ -66,6 +65,10 @@ fd_store_new( void * shmem, ulong fec_max, ulong part_cnt ) {
   store->root     = null;
 
   fd_store_pool_t pool = fd_store_pool( store );
+  if( FD_UNLIKELY( !fd_store_pool_new( shpool ) ) ) {
+    FD_LOG_WARNING(( "fd_store_pool_new failed" ));
+    return NULL;
+  }
   fd_store_pool_reset( &pool, 0 );
 
   FD_COMPILER_MFENCE();
@@ -115,7 +118,8 @@ fd_store_leave( fd_store_t const * store ) {
 }
 
 void *
-fd_store_delete( void * store ) {
+fd_store_delete( void * shstore ) {
+  fd_store_t * store = shstore;
 
   if( FD_UNLIKELY( !store ) ) {
     FD_LOG_WARNING(( "NULL store" ));
@@ -127,6 +131,15 @@ fd_store_delete( void * store ) {
     return NULL;
   }
 
+  if( FD_UNLIKELY( store->magic!=FD_STORE_MAGIC ) ) {
+    FD_LOG_WARNING(( "bad magic" ));
+    return NULL;
+  }
+
+  FD_COMPILER_MFENCE();
+  FD_VOLATILE( store->magic ) = 0UL;
+  FD_COMPILER_MFENCE();
+
   return store;
 }
 
@@ -207,14 +220,14 @@ fd_store_publish( fd_store_t  * store,
      any descendants of those ancestors. */
 
   while( FD_LIKELY( head ) ) {
-    fd_store_fec_t * child = fd_store_pool_ele( &pool, head->child );          /* left-child */
+    fd_store_fec_t * child = fd_store_pool_ele( &pool, head->child );         /* left-child */
     while( FD_LIKELY( child ) ) {                                             /* iterate over children */
       if( FD_LIKELY( child != newr ) ) {                                      /* stop at new root */
         tail->next = fd_store_map_idx_remove( map, &child->key, null, fec0 ); /* remove node from map to reuse `.next` */
-        tail       = fd_store_pool_ele( &pool, tail->next );                   /* push onto BFS queue (so descendants can be pruned) */
+        tail       = fd_store_pool_ele( &pool, tail->next );                  /* push onto BFS queue (so descendants can be pruned) */
         tail->next = null;                                                    /* clear map next */
       }
-      child = fd_store_pool_ele( &pool, child->sibling );                      /* right-sibling */
+      child = fd_store_pool_ele( &pool, child->sibling );                     /* right-sibling */
     }
     fd_store_fec_t * next = fd_store_pool_ele( &pool, head->next ); /* pophead */
     int err = fd_store_pool_release( &pool, head, BLOCKING );       /* release */
@@ -231,12 +244,18 @@ fd_store_publish( fd_store_t  * store,
 
 fd_store_t *
 fd_store_clear( fd_store_t * store ) {
+
   fd_store_map_t * map  = fd_store_map( store );
   fd_store_pool_t  pool = fd_store_pool( store );
   fd_store_fec_t * fec0 = fd_store_fec0( store );
 
   fd_store_fec_t * head = fd_store_root( store );
   fd_store_fec_t * tail = head;
+
+# if FD_STORE_USE_HANDHOLDING
+  if ( FD_UNLIKELY( !head ) ) { FD_LOG_WARNING(( "calling clear on an empty store" )); return store; }
+# endif
+
   for( fd_store_map_iter_t iter = fd_store_map_iter_init( map, fec0 );
        !fd_store_map_iter_done( iter, map, fec0 );
        iter = fd_store_map_iter_next( iter, map, fec0 ) ) {
@@ -283,6 +302,8 @@ fd_store_verify( fd_store_t * store ) {
       return -1;
     }
   }
+  fd_store_pool_t pool = fd_store_pool_const( store );
+  if( FD_UNLIKELY( fd_store_pool_verify( &pool )==-1 ) ) return -1;
   return fd_store_map_verify( map, store->fec_max, fec0 );
 }
 
@@ -296,7 +317,7 @@ print( fd_store_t const * store, fd_store_fec_t const * fec, int space, const ch
 
   if( space > 0 ) printf( "\n" );
   for( int i = 0; i < space; i++ ) printf( " " );
-  printf( "%s%s", prefix, FD_BASE58_ENC_32_ALLOCA( &fec->key ) );
+  printf( "%s%s", prefix, FD_BASE58_ENC_32_ALLOCA( &fec->key.mr ) );
 
   fd_store_fec_t const * curr = fd_store_pool_ele_const( &pool, fec->child );
   char new_prefix[1024]; /* FIXME size this correctly */

src/disco/store/fd_store.h

@@ -94,7 +94,7 @@
    #define MAP_KEY_HASH(key,seed) ((ulong)key->mr.ul[0]%seed + (key)->part*seed)
    ```
    where `key` is a key type that includes the merkle root (32 bytes)
-   and the partition index (4 bytes) that is equivalent to the Shred
+   and the partition index (8 bytes) that is equivalent to the Shred
    tile index doing the insertion.  seed, on initialization, is the
    number of chains/buckets in the map_chain divided by the number of
    partitions.  In effect, seed is the size of each partition.  For
@@ -126,7 +126,7 @@
    hash chain within that slot (which modifies what the head of the
    chain points to as well as the now-previous head in the hash chain's
    `.next` field, but does not touch application data).  With fencing
-   enabled (FD_MAP_INSERT_FENCING), it is guaranteed the consumer either
+   enabled (MAP_INSERT_FENCE), it is guaranteed the consumer either
    reads the head before or after the update.  If it reads before, that
    is safe, it would just check the key (if no match, iterate down the
    chain etc.)  If it reads after, it is also safe because the new
@@ -171,15 +171,15 @@
 #define FD_STORE_DATA_MAX (63985UL) /* TODO fixed-32 */
 
 /* fd_store_fec describes a store element (FEC set).  The pointer fields
-   implement a left-child, right-sibling n-ary tree. */
+   implement a left-child, right-sibling n-ary tree. */
 
 struct __attribute__((packed)) fd_store_key {
    fd_hash_t mr;
    ulong     part; /* partition index of the inserter */
 };
 typedef struct fd_store_key fd_store_key_t;
 
-struct __attribute__((aligned(128UL))) fd_store_fec {
+struct __attribute__((aligned(FD_STORE_ALIGN))) fd_store_fec {
 
   /* Keys */
 
@@ -233,7 +233,7 @@ FD_PROTOTYPES_BEGIN
 
 /* fd_store_{align,footprint} return the required alignment and
    footprint of a memory region suitable for use as store with up to
-   fec_max elements. */
+   fec_max elements.  fec_max is an integer power-of-two. */
 
 FD_FN_CONST static inline ulong
 fd_store_align( void ) {
@@ -242,6 +242,7 @@ fd_store_align( void ) {
 
 FD_FN_CONST static inline ulong
 fd_store_footprint( ulong fec_max ) {
+  if( FD_UNLIKELY( !fd_ulong_is_pow2( fec_max ) ) ) return 0UL;
   return FD_LAYOUT_FINI(
     FD_LAYOUT_APPEND(
     FD_LAYOUT_APPEND(
@@ -257,7 +258,8 @@ fd_store_footprint( ulong fec_max ) {
 
 /* fd_store_new formats an unused memory region for use as a store.
    mem is a non-NULL pointer to this region in the local address space
-   with the required footprint and alignment. */
+   with the required footprint and alignment.  fec_max is an integer
+   power-of-two. */
 
 void *
 fd_store_new( void * shmem, ulong fec_max, ulong part_cnt );
@@ -286,7 +288,7 @@ fd_store_leave( fd_store_t const * store );
    caller. */
 
 void *
-fd_store_delete( void * store );
+fd_store_delete( void * shstore );
 
 /* Accessors */
 
@@ -451,7 +453,9 @@ fd_store_publish( fd_store_t * store,
                   fd_hash_t  * merkle_root );
 
 /* fd_store_clear clears the store.  All elements are removed from the
-   map and released back into the pool.  Does not zero-out fields. */
+   map and released back into the pool.  Does not zero-out fields.
+
+   IMPORTANT SAFETY TIP!  the store must be non-empty. */
 
 fd_store_t *
 fd_store_clear( fd_store_t * store );