- Replace all cryptonote_config macros with constexpr variables. Some
become integer types, some become chrono types.
- generally this involved removing a "CRYPTONOTE_" prefix since the
values are now in the `cryptonote` namespace
- some constants are grouped into sub-namespaces (e.g.
cryptonote::p2p)
- deprecated constants (i.e. for old HFs) are in the `cryptonote::old`
namespace.
- all the magic hash key domain separating strings are now in
cryptonote::hashkey::WHATEVER.
- Move some economy-related constants to oxen_economy.h instead
- Replaced the BLOCKS_EXPECTED_IN_DAYS constexpr functions with more
straightforward `BLOCKS_PER_DAY` value (i.e. old
`BLOCKS_EXPECTED_IN_DAYS(10)` is now `BLOCKS_PER_DAY * 10`.
- Replaced `network_version` unscoped enum with a scoped enum
`cryptonote::hf`, replacing all the raw uint8_t values where it was
currently accepted with the new `hf` type.
- Made `network_type` a scoped enum so that it now has to be qualified
(network_type::TESTNET) and can't be arbitrarily/unintentionally
converted to/from an int.
- HARDFORK_WHATEVER macros have become cryptonote::feature::WHATEVER
constexpr hf values.
- Add `revision` to rpc hard_fork_info response
- Don't build trezor code at all (previously we were pointlessly
building an empty dummy lib).
Not only is this endpoint not used anywhere, but it returns (invalid)
cancerous json output when invoked in current stable oxend:
{
"jsonrpc": "2.0",
"id": "0",
"result": {
"backlog": "l\u0007\u0000\u0000\u0000\u0000\u0000\u0000�\u001f�\u0000\u0000\u0000\u0000\u0000Y�������",
"status": "OK",
"untrusted": false
}
}
where that cancer in the backlog field is the raw bytes copied out from
under a struct with 3 uint64_t's. (It's also invalid json because those
unprintable characters are \xff, which isn't valid UTF-8 encoding, and
thus isn't valid JSON).
Just nuke the whole thing since nothing uses it (probably because it is
such cancer).
This drops the dedicated GET_TRANSACTION_POOL endpoint entirely and
folds the txpool fetching into GET_TRANSACTIONS via a new `memory_pool`
parameter. (This is backwards incompatible, of course, but the results
may not be too harmful: the wallet doesn't actually use this endpoint,
so it mainly affects internal oxend commands (fixed here) and the block
explorer (which is fairly easily fixed).
This also removes some of the useless GET_TRANSACTIONS bits such as
decode_as_json.
`fee` and `burned` are now always provided in the return (for both
specific requested transactions and mempool transactions).
As a side effect, this code ventured deep into core/blockchain in terms
of dealing with missed transactions: previously they were always
returned (via lvalue ref) as a vector, and always had to be specified.
This changes to take via pointer to an unordered_set, which can be null
if you don't care about the missed ones. This reduces several calls
that indeed didn't care about collecting the missed values, and improved
virtually all the places that *did* care which need to query which ones
were missed rather than needing a specific order.
- Take the "include_unrelayed" as a parameter instead of switching on on
the admin context; there isn't likely to be anything sensitive there,
and it makes little sense for a wallet to get *different* results just
because it happens to be talking to a localhost oxend.
- Change the histogram code so that it always returns a fixed size
- Various improvements to histogram output; most notably printing a more
useful indication of what the histogram outputs are (e.g. instead of:
Age Txes Bytes
now 0 0
8.0 minutes ago 0 0
16.1 minutes ago 0 0
24.1 minutes ago 0 0
32.2 minutes ago 0 0
40.2 minutes ago 0 0
48.3 minutes ago 0 0
56.3 minutes ago 0 0
64.3 minutes ago 0 0
72.4 minutes ago 2 4559
it now displays the ranges:
Age Txes Bytes
now - 10.6min ago 0 0
10.6min - 21.3min ago 0 0
21.3min - 32.0min ago 0 0
32.0min - 42.6min ago 0 0
42.6min - 53.2min ago 0 0
53.2min - 63.9min ago 0 0
63.9min - 74.6min ago 0 0
74.6min - 85.2min ago 0 0
85.2min - 1.6hr ago 0 0
1.6hr - 1.8hr ago 2 4559
It makes no sense for the definition to be there (except that it was
for epee serialization purposes), so move it into tx_memory_pool where
it belongs, and convert it to json on the fly for the rpc method that
wants it.
This json serialization layer was only used in the old Monero ZMQ
interface, which no longer exists, and so this is just dead code.
On top of that, it doesn't work properly for serializing CLSAG
transactions, so just delete it.
Converts all use of boost::filesystem to std::filesystem.
For macos and potentially other exotic systems where std::filesystem
isn't available, we use ghc::filesystem instead (which is a drop-in
replacement for std::filesystem, unlike boost::filesystem).
This also greatly changes how we handle filenames internally by holding
them in filesystem::path objects as soon as possible (using
fs::u8path()), rather than strings, which avoids a ton of issues around
unicode filenames. As a result this lets us drop the boost::locale
dependency on Windows along with a bunch of messy Windows ifdef code,
and avoids the need for doing gross boost locale codecvt calls.
- Sometimes the wallet would query TX's before they are blink approved
and get cached into the wallet as a normal TX preventing external
services from using 0-confirmation on transactions.
We make the wallet only conduct long-polling for blinks in order to
notice the transaction after blink. The downside here is that normal
mempool transacations won't appear until they are placed into a block.
With Pulse this is mitigated with reliable within sub-seconds of
a target 2 min block time.
- The fee returned from the function is expected to be the raw_fee
(i.e the sum of all transactions included without any penalty applied to
the fee), as interacting functions re-calculate the reward using the raw
fee.
This purges epee::critical_region/epee::critical_section and the awful
CRITICAL_REGION_LOCAL and CRITICAL_REGION_LOCAL1 and
CRITICAL_REGION_BEGIN1 and all that crap from epee code.
This wrapper class around a mutex is just painful, macro-infested
indirection that accomplishes nothing (and, worse, forces all using code
to use a std::recursive_mutex even when a different mutex type is more
appropriate).
This commit purges it, replacing the "critical_section" mutex wrappers
with either std::mutex, std::recursive_mutex, or std::shared_mutex as
appropriate. I kept anything that looked uncertain as a
recursive_mutex, simple cases that obviously don't recurse as
std::mutex, and simple cases with reader/writing mechanics as a
shared_mutex.
Ideally all the recursive_mutexes should be eliminated because a
recursive_mutex is almost always a design flaw where someone has let the
locking code get impossibly tangled, but that requires a lot more time
to properly trace down all the ways the mutexes are used.
Other notable changes:
- There was one NIH promise/future-like class here that was used in
example one place in p2p/net_node; I replaced it with a
std::promise/future.
- moved the mutex for LMDB resizing into LMDB itself; having it in the
abstract base class is bad design, and also made it impossible to make a
moveable base class (which gets used for the fake db classes in the test
code).
Changes all boost mutexes, locks, and condition_variables to their stl
equivalents.
Changes all lock_guard/unique_lock/shared_lock to not specify the mutex
type (C++17), e.g.
std::lock_guard foo{mutex};
instead of
std::lock_guard<oh::um::what::mutex> foo{mutex};
Also changes some related boost::thread calls to std::thread, and some
related boost chrono calls to stl chrono.
boost::thread isn't changed here to std::thread because some of the
instances rely on some boost thread extensions.
High-level details:
This redesigns the RPC layer to make it much easier to work with,
decouples it from an embedded HTTP server, and gets the vast majority of
the RPC serialization and dispatch code out of a very commonly included
header.
There is unfortunately rather a lot of interconnected code here that
cannot be easily separated out into separate commits. The full details
of what happens here are as follows:
Major details:
- All of the RPC code is now in a `cryptonote::rpc` namespace; this
renames quite a bit to be less verbose: e.g. CORE_RPC_STATUS_OK
becomes `rpc::STATUS_OK`, and `cryptonote::COMMAND_RPC_SOME_LONG_NAME`
becomes `rpc::SOME_LONG_NAME` (or just SOME_LONG_NAME for code already
working in the `rpc` namespace).
- `core_rpc_server` is now completely decoupled from providing any
request protocol: it is now *just* the core RPC call handler.
- The HTTP RPC interface now lives in a new rpc/http_server.h; this code
handles listening for HTTP requests and dispatching them to
core_rpc_server, then sending the results back to the caller.
- There is similarly a rpc/lmq_server.h for LMQ RPC code; more details
on this (and other LMQ specifics) below.
- RPC implementing code now returns the response object and throws when
things go wrong which simplifies much of the rpc error handling. They
can throw anything; generic exceptions get logged and a generic
"internal error" message gets returned to the caller, but there is
also an `rpc_error` class to return an error code and message used by
some json-rpc commands.
- RPC implementing functions now overload `core_rpc_server::invoke`
following the pattern:
RPC_BLAH_BLAH::response core_rpc_server::invoke(RPC_BLAH_BLAH::request&& req, rpc_context context);
This overloading makes the code vastly simpler: all instantiations are
now done with a small amount of generic instantiation code in a single
.cpp rather than needing to go to hell and back with a nest of epee
macros in a core header.
- each RPC endpoint is now defined by the RPC types themselves,
including its accessible names and permissions, in
core_rpc_server_commands_defs.h:
- every RPC structure now has a static `names()` function that returns
the names by which the end point is accessible. (The first one is
the primary, the others are for deprecated aliases).
- RPC command wrappers define their permissions and type by inheriting
from special tag classes:
- rpc::RPC_COMMAND is a basic, admin-only, JSON command, available
via JSON RPC. *All* JSON commands are now available via JSON RPC,
instead of the previous mix of some being at /foo and others at
/json_rpc. (Ones that were previously at /foo are still there for
backwards compatibility; see `rpc::LEGACY` below).
- rpc::PUBLIC specifies that the command should be available via a
restricted RPC connection.
- rpc::BINARY specifies that the command is not JSON, but rather is
accessible as /name and takes and returns values in the magic epee
binary "portable storage" (lol) data format.
- rpc::LEGACY specifies that the command should be available via the
non-json-rpc interface at `/name` for backwards compatibility (in
addition to the JSON-RPC interface).
- some epee serialization got unwrapped and de-templatized so that it
can be moved into a .cpp file with just declarations in the .h. (This
makes a *huge* difference for core_rpc_server_commands_defs.h and for
every compilation unit that includes it which previously had to
compile all the serialization code and then throw all by one copy away
at link time). This required some new macros so as to not break a ton
of places that will use the old way putting everything in the headers;
The RPC code uses this as does a few other places; there are comments
in contrib/epee/include/serialization/keyvalue_serialization.h as to
how to use it.
- Detemplatized a bunch of epee/storages code. Most of it should have
have been using templates at all (because it can only ever be called
with one type!), and now it isn't. This broke some things that didn't
properly compile because of missing headers or (in one case) a messed
up circular dependency.
- Significantly simplified a bunch of over-templatized serialization
code.
- All RPC serialization definitions is now out of
core_rpc_server_commands_defs.h and into a single .cpp file
(core_rpc_server_commands_defs.cpp).
- core RPC no longer uses the disgusting
BEGIN_URI_MAP2/MAP_URI_BLAH_BLAH macros. This was a terrible design
that forced slamming tons of code into a common header that didn't
need to be there.
- epee::struct_init is gone. It was a horrible hack that instiated
multiple templates just so the coder could be so lazy and write
`some_type var;` instead of properly value initializing with
`some_type var{};`.
- Removed a bunch of useless crap from epee. In particular, forcing
extra template instantiations all over the place in order to nest
return objects inside JSON RPC values is no longer needed, as are a
bunch of stuff related to the above de-macroization of the code.
- get_all_service_nodes, get_service_nodes, and get_n_service_nodes are
now combined into a single `get_service_nodes` (with deprecated
aliases for the others), which eliminates a fair amount of
duplication. The biggest obstacle here was getting the requested
fields reference passed through: this is now done by a new ability to
stash a context in the serialization object that can be retrieved by a
sub-serialized type.
LMQ-specifics:
- The LokiMQ instance moves into `cryptonote::core` rather than being
inside cryptonote_protocol. Currently the instance is used both for
qnet and rpc calls (and so needs to be in a common place), but I also
intend future PRs to use the batching code for job processing
(replacing the current threaded job queue).
- rpc/lmq_server.h handles the actual LMQ-request-to-core-RPC glue.
Unlike http_server it isn't technically running the whole LMQ stack
from here, but the parallel name with http_server seemed appropriate.
- All RPC endpoints are supported by LMQ under the same names as defined
generically, but prefixed with `rpc.` for public commands and `admin.`
for restricted ones.
- service node keys are now always available, even when not running in
`--service-node` mode: this is because we want the x25519 key for
being able to offer CURVE encryption for lmq RPC end-points, and
because it doesn't hurt to have them available all the time. In the
RPC layer this is now called "get_service_keys" (with
"get_service_node_key" as an alias) since they aren't strictly only
for service nodes. This also means code needs to check
m_service_node, and not m_service_node_keys, to tell if it is running
as a service node. (This is also easier to notice because
m_service_node_keys got renamed to `m_service_keys`).
- Added block and mempool monitoring LMQ RPC endpoints: `sub.block` and
`sub.mempool` subscribes the connection for new block and new mempool
TX notifications. The latter can notify on just blink txes, or all
new mempool txes (but only new ones -- txes dumped from a block don't
trigger it). The client gets pushed a [`notify.block`, `height`,
`hash`] or [`notify.tx`, `txhash`, `blob`] message when something
arrives.
Minor details:
- rpc::version_t is now a {major,minor} pair. Forcing everyone to pack
and unpack a uint32_t was gross.
- Changed some macros to constexprs (e.g. CORE_RPC_ERROR_CODE_...).
(This immediately revealed a couple of bugs in the RPC code that was
assigning CORE_RPC_ERROR_CODE_... to a string, and it worked because
the macro allows implicit conversion to a char).
- De-templatizing useless templates in epee (i.e. a bunch of templated
types that were never invoked with different types) revealed a painful
circular dependency between epee and non-epee code for tor_address and
i2p_address. This crap is now handled in a suitably named
`net/epee_network_address_hack.cpp` hack because it really isn't
trivial to extricate this mess.
- Removed `epee/include/serialization/serialize_base.h`. Amazingly the
code somehow still all works perfectly with this previously vital
header removed.
- Removed bitrotted, unused epee "crypted_storage" and
"gzipped_inmemstorage" code.
- Replaced a bunch of epee::misc_utils::auto_scope_leave_caller with
LOKI_DEFERs. The epee version involves quite a bit more instantiation
and is ugly as sin. Also made the `loki::defer` class invokable for
some edge cases that need calling before destruction in particular
conditions.
- Moved the systemd code around; it makes much more sense to do the
systemd started notification as in daemon.cpp as late as possible
rather than in core (when we can still have startup failures, e.g. if
the RPC layer can't start).
- Made the systemd short status string available in the get_info RPC
(and no longer require building with systemd).
- during startup, print (only) the x25519 when not in SN mode, and
continue to print all three when in SN mode.
- DRYed out some RPC implementation code (such as set_limit)
- Made wallet_rpc stop using a raw m_wallet pointer
This reduces blink fees by half (from 5x to 2.5x base fee) at HF15, and
makes anything other than "unimportant" priority map to blink for
ordinary transactions.
For non-blink txes priorities are still accepted (so that, if the
mempool is clogged, you can still get a registration or stake through by
upping the priority).
It also updates the wallets default priority for transactions to blink
(that is, "default" now becomes blink).
With the priorities gone and default set to blink, the backlog-checking
code and automatic priority bumping code don't serve any useful purpose,
so this rips them out, along with a few other related code
simplifications.
cryptonote_protocol_handler calls `pool.get_blink(hash)` while already
holding a blink shared lock, which should have been
`pool.get_blink(hash, true)` to avoid `get_blink` trying to take its own
lock.
That double lock is undefined behaviour and can cause a deadlock on the
mutex, although it appears rare that it actually does. If it does,
however, this eventually backs up into vote relaying during the idle
loop, which then stalls the idle loop so we stop sending out uptime
proofs (since that is also in the idle loop).
A simple fix here is to add the `true` argument, but on reconsideration
this extra argument to take or not take a lock is messy and error prone,
so this commit instead removes the second argument entirely and instead
documents which call must and must not hold a lock, getting rid of the
three methods (get_blink, has_blink, and add_existing_blink) that had
the `have_lock` argument. This ends up having only a small impact on
calling code - the vast majority of callers already hold a lock, and the
few that don't are easily adjusted.
This replaces the horrible, horrible, badly misused templated
once_a_time_seconds and once_a_time_milliseconds with a `periodic_task`
that works the same way but takes parameters as constructor arguments
instead of template parameters.
It also makes various small improvements:
- uses std::chrono::steady_clock instead of ifdef'ing platform dependent
timer code.
- takes a std::chrono duration rather than a template integer and
scaling parameter.
- timers can be reset to trigger on the next invocation, and this is
thread-safe.
- timer intervals can be changed at run-time.
This all then gets used to reset the proof timer immediately upon
receiving a ping (initially or after expiring) from storage server and
lokinet so that we send proofs out faster.
Blockchain::prepare_handle_incoming_blocks locks m_tx_pool, but uses a
local RAII lock on the blockchain object itself, then also starts a
batch. Blockchain::cleanup_handle_incoming_blocks then also takes out a
local RAII blockchain lock, then cleans up the batch.
But the lmdb layer is retarded in that it throws an exception if any
thread tries to write to the database while a batch is active in another
thread, and so the blockchain lock is *also* used as a guard writes.
Holding an open batch but *not* holding the blockchain lock then hits
this exception if a write arrives in another thread at just the right
time.
This is, of course, terrible design at multiple layers, but this close
to release I am reluctant to make more drastic fixes.
Other small changes here:
- All the locks in `blockchain.cpp` now use tools::unique_lock or
tools::unique_locks rather than the nasty epee macro. This also
reduces the likelihood of accidental deadlock because this means the
dual txpool-blockchain locks are not taken out simultaneously via
std::lock rather than sequentially.
- Removed a completely useless "if (1)". Git blame shows that there was
previously a condition here, but apparently the upstream monero author
who changed it was afraid of removing the `if` keyword.
- Reduced the sleep in the loop that waits for a batch to 100ms from
1000ms because sleeping for a full second for a fairly light test is
insane.
- boost isn't happy calling boost::lock() on the tx pool or blockchain
object because the lock/unlock/try_lock methods are const, and so the
workaround of using boost::lock because std::lock and
std::shared_time_mutex are broken on the macOS SDK 10.11 that we use
for mac builds now requires extra workarounds. Joy.
The MacOSX 10.11 SDK we use is broken AF: it lies about supporting
C++14, but really only upgraded the headers but not the library itself,
so using std::shared_timed_mutex just results in a linking failure.
Upgrading the SDK is a huge pain (I tried, failed, and gave up), so for
now temporarily switch to boost::shared_mutex until we sort out the
macOS build disaster.
This adds support for bumping conflicting transactions from the mempool
upon receipt of a blink tx if those conflicting txs are not themselves
blinks.
This commit chops the handle_incoming_txs effectively in half into a
parse_incoming_txs phase, followed by a handle_incoming_txs phase which
is needed so that we can first parse the txes then, assuming they parse
successfully, can flag them as blink txes if blink info was included
with them, so that the actual insertion becomes forcing.
There is a stub here for also allowing a rollback of the mutable part of
the chain; implementation of actually doing that rollback will follow in
another commit.
To make it obvious (and keep it consistent) with all the other
blink-related methods having "blink" in their name.
Also fix grammar in a related debug message.
Extracts the tx removal lambda into a new method.
DRYs the nearly identical prune loop bodies (the only difference between
them is the iteration order).
Fix undefined behaviour in mempool pruning: on removal of a standard tx
the iterator was updated to the result of `.erase()`, but that's wrong
because the loop is going in backwards order, and so the first prune
would leave the iterator pointing at `.end()` which would be deferenced
in the next iteration. The (new, DRY) lambda now iterates in the
desired direction.
This also makes (and uses) blockchain and tx_pool implement the standard
library Lockable concept by adding a `try_lock()` so that they can be
locked simultaneously. It also updates them to hold a
boost::recursive_mutex directly rather than going through the pointless
and horribly designed epee `critical_section` wrapper (seriously who
thinks that making a mutex class copyable where a copy holds a new,
entirely unrelated mutex is a good idea?)
This makes get_blink_hashes_and_mined_heights filter out blinks in
immutable blocks; since we have to find them all here in order to do
that (and because this is regularly called) it's also a good place to
expire old blink entries.
- actually include the blink hashes in the core sync data
- fix cleanup to delete heights in (0, immutable] instead of [0,
immutable); we want to keep 0 because it signifies the mempool, and we
only need blocks after immutable, not immutable itself.
- fixed NOTIFY_REQUEST_GET_TXS to handle mempool txes properly (it was
only searching the chain and ignored missed_txs, but missed_txs are ones
we need to look up in the mempool)
- Add a method to tx_pool (needed for the above) to grab multiple txes
by hash (essentially a multi-tx version of `get_transaction()`), and
change get_transaction() to use it with a single value.
- Added various debugging statements
- Added a bunch of comments to each condition of the preliminary blink
data check condition.
- Don't abort blink addition on a single signature failure: if there are
enough valid signatures we should still accept it.
- Check for blink signature approval when receiving blink signatures;
it's not enough to know that all were added successfully, we also have
to ask the blink tx if it is approved (which does additional checks on
subquorum counts) once we add them all.
This adds blink tx synchronization: when doing a periodic sync with
other nodes each node includes a map of {HEIGHT, HASH} pairs where
HEIGHT is a mined, post-immutable block height and HASH is an xor of all
the tx hashes mined into that block.
If upon receipt the node disagrees about what it thinks the HASH should
be, it can request a list of txes for one or more disagreeing heights,
for which it gets a list of tx hashes of all blink txes mined into that
block.
If it is then missing any of those TXes, this adds an ability to request
that the remote send TXes via the existing NOTIFY_NEW_TRANSACTIONS
mechanism, but with an added flag (so that these don't get relayed).
This originally was going to request the TXes via the existing
NOTIFY_REQUEST_GET_OBJECTS, which has a `txs` field, but unfortunately
any txs passed to it are completely ignored; it is *only* usable for
block synchronization. As such I renamed it and removed the `txs` field
to make the responsibility/capability clearer.
This adds the ability for check_fee() to also check the burn amount.
This requires passing extra info through `add_tx()` (and the various
things that call it), so I took the:
bool keeped_by_block, bool relayed, bool do_not_relay
argument triplet, moved it into a struct in tx_pool.h, then added the other fee
options there (along with some static factory functions for generating the
typical sets of option).
The majority of this commit is chasing that change through the codebase and
test suite.
This is used by blink but should also help LNS and other future burn
transactions to verify a burn amount simply when adding the transation to the
mempool. It supports a fixed burn amount, a burn amount as a multiple of the
minimum tx fee, and also allows you to increase the minimum tx fee (so that,
for example, we could require blink txes to pay miners 250% of the usual
minimum (unimportant) priority tx fee.
- Removed a useless core::add_new_tx() overload that wasn't used anywhere.
Blink-specific changes:
(I'd normally separate these into a separate commit, but they got interwoven
fairly heavily with the above change).
- changed the way blink burning is specified so that we have three knobs for
fee adjustment (fixed burn fee; base fee multiple; and required miner tx fee).
The fixed amount is currently 0, base fee is 400%, and require miner tx fee is
simply 100% (i.e. no different than a normal transaction). This is the same as
before this commit, but is changing how they are being specified in
cryptonote_config.h.
- blink tx fee, burn amount, and miner tx fee (if > 100%) now get checked
before signing a blink tx. (These fee checks don't apply to anyone else --
when propagating over the network only the miner tx fee is checked).
- Added a couple of checks for blink quorums: 1) make sure they have reached
the blink hf; 2) make sure the submitted tx version conforms to the current hf
min/max tx version.
- print blink fee information in simplewallet's `fee` output
- add "typical" fee calculations in the `fee` output:
[wallet T6SCwL (has locked stakes)]: fee
Current fee is 0.000000850 loki per byte + 0.020000000 loki per output
No backlog at priority 1
No backlog at priority 2
No backlog at priority 3
No backlog at priority 4
Current blink fee is 0.000004250 loki per byte + 0.100000000 loki per output
Estimated typical small transaction fees: 0.042125000 (unimportant), 0.210625000 (normal), 1.053125000 (elevated), 5.265625000 (priority), 0.210625000 (blink)
where "small" here is the same tx size (2500 bytes + 2 outputs) used to
estimate backlogs.
- Adds blink signature synchronization and storage through the regular
p2p network
- Adds wallet support (though this is still currently buggy and needs
additional fixes - it sees the tx when it arrives in the mempool but
isn't properly updating when the blink tx gets mined.)
Pool printing is separately implemented in rpc_command_executor; this
method in tx_pool is both obsolete (missing some fields) and not called
anywhere.
