[net] [validation] Call ProcessNewBlock() asynchronously #12934

skeees · 2018-04-10T21:54:36Z

Update

I think this is now in a state for code review
Summary of discussion of overall design as well as some concept acks: https://bitcoincore.org/en/meetings/2018/05/03/

Description

~~This is still in progress and not fully completed yet, but wanted to put it out for review in terms of overall design/architecture~~

The high level goal here (in this, and if accepted, subsequent PRs) is to allow for net and validation to live on separate threads and communicate mostly via message passing - both for the efficiency benefits that further parallelism in the net layer might provide, but also perhaps moreso as a step towards the goal of reducing the amount of shared state and forcing a cleaner separation between the net and validation layers in the core node.

To keep this PR as self contained as possible - this set of commits does the following:

defines ProducerConsumerQueue() / ConsumerThread(): infrastructure to facilitate async communication between the net and validation layers
defines ValidationLayer(): an interface where requests for (just CBlock for now) validation can be submitted and processed asynchronously
replaces synchronous calls of ProcessNewBlock() in net_processing with the new async interface ValidationLayer::SubmitForValidation(CBlock) -> std::future<BlockValidationResult>

Because the P2P layer assumes that for a given node every message is fully processed before any subsequent messages are processed, when an asynchronous validation request is submitted for a block coming from a node - that node is "frozen" until that request has been fully validated. In the meantime - the net layer may continue servicing other nodes that do not have pending asynchronous validation requests.

The ProducerConsumerQueue() was left sufficiently generic so that it may be interposed in other places where separation of components via asynchronous message passing might make sense from a design perspective.

fanquake · 2018-04-11T04:28:02Z

cc @theuni
@skeees Have you looked through the current work being done to refactor the P2P code? See here for an overview.

skeees · 2018-04-11T13:27:26Z

Thanks, yes I have looked through those. This is more focused on separation between net_processing (PeerLogicValidation) and validation, whereas those primarily tackle socket handling and other ConnMan stuff. I don't think there's anything here that's redundant or incompatible with those refactors

instagibbs · 2018-04-13T02:26:09Z

cc @TheBlueMatt

ryanofsky

@TheBlueMatt, could you take a quick look at the last commit ("Call ProcessNewBlock() asynchronously in a separate thread"), and give a concept ACK/NACK if you think the approach makes sense? (All the commits before the last one are pretty straightforward util code.)

It seems like a clean change that disentangles network & validation code and could make bitcoin more responsive to other network requests when blocks are coming in.

ryanofsky · 2018-04-23T18:44:01Z

src/core/consumerthread.h

I think you could replace this class with using WorkItem = std::function<void()>;. Less code and would make queue interface more generic.

ryanofsky · 2018-04-23T18:50:50Z

src/core/consumerthread.h

Could maybe use TraceThread from util.h to make the thread name visible to the os.

ryanofsky · 2018-04-23T18:58:05Z

src/core/consumerthread.h

ShutdownPill seems a little complicated. What advantages does it provide over just adding bool m_active to ProducerConsumerQueue with a simple method to set it to false and cancel blocked Pop() calls?

Yeah - it is pretty complicated. I didn't want to poke through the queue api just to enable shutdown - you'd have to have T Pop() potentially not return a T (i.e. throw an exception) - which seemed less desirable and maybe equally complicated.

Having said that - a lot of the complexity here is introduced to handle:

being able to shutdown a specific ConsumerThread without shutting down others (in reality you probably only ever want to shut down all of them when you terminate the process)

allowing for a queue with fewer slots than the number of threads servicing it (unlikely)

Only reason I allowed for these is so that the APIs to the queue work the way they sound like they should work and for unit test completeness. If I discard the above the code gets much simpler and it will just be broken for two use cases that seem pretty unlikely to ever happen right now (but you never know - and then maybe somebody gets frustrated one day).

ryanofsky · 2018-04-23T19:09:03Z

src/validation_layer.h

Could use make_shared here, MakeUnique below

ryanofsky · 2018-04-23T19:13:22Z

src/validation_layer.h

Probably should take unique_ptr instead of raw argument to clarify ownership

skeees · 2018-04-23T22:52:16Z

Thank you for the review - one thing (general design related) to add to the discussion here:

Since I've submitted this request - I happened to stumble upon two race conditions in validation that stem from concurrent calls to ProcessNewBlock (#12988, #13023)
This pr should simplify the concurrency model for block validation (a single validation thread pulls a block to validate from the queue and validates it completely before moving on to the next block) and would have inadvertently fixed those two referenced race conditions.

Explicitly simplifying the concurrency model hopefully reduces a bit the cognitive burden of future code changes in validation and I don't think makes anything substantially less efficient - much of validation is already single threaded (because of cs_main), and certain pieces fundamentally cannot be concurrent (i.e. connecttip). Validation is already complicated enough to understand on its own without worrying about concurrency.

Seems like the clarity gains will outweigh the minor efficiency hit here - +the async api into should allow all the stuff around validation to be more easily be parallelized with less risk of inadvertently introducing a consensus bug. And it makes process separation / alternate p2p more natural if that's ever to be a thing in the future.

If this design seems useful - my intention is to finish this pr up (some stuff around compact blocks that I still have to work through + refit the couple of places in rpc that call ProcessNewBlock) and explore subsequent prs to put a similar model in place around the mempool. I'd also like to explore feasibility for header processing.

TheBlueMatt · 2018-04-28T01:57:33Z

I do think the general approach is fine. It's not going to be really at all useful until we do a ton of locking cleanups in net_processing (it'll let us run ahead to the next message, which almost always requires cs_main, and we'll end up blocking on validation anyway). It's probably simpler than multiple net_processing threads (with the same cleanups required) which was most of my previous work, but they'll end up looking pretty similar on the net_processing end, we were gonna need this blocking-on-response logic either way. Should definitely get brought up at a meeting, though, to get wider feedback. @theuni probably has some thoughts, too.

skeees · 2018-06-05T11:54:33Z

PR updated with latest commits, ready for review
Also, for reference, a discussion on high level design for this PR at the IRC meeting a couple weeks ago:
https://bitcoincore.org/en/meetings/2018/05/03/

ryanofsky

(edited 2018-08-16)

utACK 3d6f038. Code looks good, and from the linked IRC discussion, this seems like a promising direction to move in. All of my comments are just suggestions which you should feel free to ignore.

c6396d9 Implement a thread-safe FIFO (producer/consumer style) queue (1/9)
3f09adf Unit tests for ProducerConsumerQueue (2/9)
7f8a888 Add ConsumerThread: to consumer and operate on work from a ProducerConsumerQueue (3/9)
134d47a ConsumerThread unit tests (4/9)
b471728 ValidationLayer() - interface for calls into block validation (5/9)
1c9f741 Call ProcessNewBlock() asynchronously in a separate thread from p2p layer (6/9)
ce1d845 Replace all instances of ProcessNewBlock() with ValidationLayer.Validate() (7/9)
3778736 Limit available scope of ProcessNewBlock to ValidationLayer (move-only) (8/9)
3d6f038 Fix whitespace in test_bitcoin.cpp (whitespace,move-only) (9/9)

ryanofsky · 2018-06-14T17:11:53Z

src/core/producerconsumerqueue.h

+                m_producer_cv.wait(l, [&]() { return m_data.size() < m_capacity; });
+            }
+
+            m_data.push_back(std::forward<TT>(data));


In commit "Implement a thread-safe FIFO (producer/consumer style) queue" (d810614)

Should probably emplace_back to avoid creating a temporary.

not sure i completely understand - data is already constructed when it is passed into the function - thought the most economical thing to do was std::forward which should use the move constructor when possible? is that different from what emplace_back would do?

Talked about this offline with @ryanofsky, @skeees; this form is equivalent (in terms of constructor calls) to what @ryanofsky initially suggested so no need to change.

ryanofsky · 2018-06-19T18:28:51Z

src/core/producerconsumerqueue.h

+ *
+ * @see WorkerMode
+ */
+template <typename T, WorkerMode m_producer_mode = WorkerMode::BLOCKING, WorkerMode m_consumer_mode = WorkerMode::BLOCKING>


In commit "Implement a thread-safe FIFO (producer/consumer style) queue" (d810614)

I don't think it's a good idea for blocking and nonblocking defaults to be attributes of the Queue data structure, instead of arguments to (or variants of) the push and pop methods. Advantages to dropping these template arguments:

Readability. It would be nice to be able to see if a push or pop call is blocking just by looking at the call, without have to check another part of the code to see how the queue data structure was initially declared.

Code size. Dropping these arguments would avoid compiler potentially having to instantiate many copies of this code for different combinations of template arguments.

Extensibility. There could be other useful blocking methods added in the future (like methods to wait for low/high water marks or for empty/full events) and it would either be verbose to have to add new classwide blocking/nonblocking defaults for new methods, or confusing to have to somehow tie existing defaults to new methods.

Consistency. If you look at other C++ objects that support optional blocking like std::mutex or std::future, the blocking behaviour is determined only by the particular method call, not by template arguments from where the object was declared.

I can see this either way - i wrote it with defaults essentially as constructor args because - at least for the use cases i can imagine you almost always want a default mode of operation for a given queue except for certain edge cases (shutdown is the most apparent one) - and i've seen data structures that handle this sort of initialization both ways (defaults on construction vs with every method call) but i'm also happy to change this up if the prevailing opinion is the other way.

Re: #12934 (comment)

I can see this either way - i wrote it with defaults essentially as constructor args because - at least for the use cases i can imagine you almost always want a default mode of operation for a given queue except for certain edge cases (shutdown is the most apparent one) - and i've seen data structures that handle this sort of initialization both ways (defaults on construction vs with every method call) but i'm also happy to change this up if the prevailing opinion is the other way.

What about having Push, Pop, TryPush, and TryPop methods and dropping the enum entirely? I think this would make code using the queue easier to understand, since blocking would be explicit, instead of based on an enum value determined by a combination of method argument, class template argument, method argument default, and class template argument default values. It could also make code using the queue easier to write, since there would no longer be any chance of hitting various compile and runtime checks for invalid enum values.

I do understand the enum is useful in tests for listing all possible combinations of behavior, but for that purpose, you could just define the enum in test code.

Readability. It would be nice to be able to see if a push or pop call is blocking just by looking at the call

I think this comment is spot on.

If you look at other C++ objects that support optional blocking like std::mutex or std::future, the blocking behaviour is determined only by the particular method call

What about having Push, Pop, TryPush, and TryPop methods and dropping the enum entirely?

Agree.

Re: #12934 (comment)

Another advantage to add to list above:

If you make blocking an attribute of push/pop methods rather than an attibutes of the queue you can drop the consumer/producer terminology, which I'm finding confusing now that I'm looking at downstream code. E.g. if I push an item into the queue, that seems like producing from my perspective, but it's consuming from the queue/worker thread perspective and makes that code a bit strange, IMO.

ryanofsky · 2018-06-19T18:32:02Z

src/core/producerconsumerqueue.h

+     */
+    T Pop()
+    {
+        static_assert(m_consumer_mode == WorkerMode::BLOCKING, "");


In commit "Implement a thread-safe FIFO (producer/consumer style) queue" (d810614)

Could return std::future<T> in the case of a non-blocking Pop() to support it instead of having this asymmetry.

Doing that would complicate internal implementation a bit - you'd have to hang on to the promise that satisfies the future internally - and either you'd have too many of those and need to block - or potentially allow your buffer holding promises to grow unbounded - so actually it might not be safely implementable.

Also I don't really see an immediate use case for this - you'd have to later wait on the future (blocking or non-blocking) - but you could just alternately wait on the queue. I can't think of any use cases right now where you'd want to reserve a place in line in a non-blocking fashion and then later claim that item.

ryanofsky · 2018-08-13T17:43:58Z

src/Makefile.am

  policy/policy.h \
  policy/rbf.h \
  pow.h \
+  core/producerconsumerqueue.h \


In commit "Implement a thread-safe FIFO (producer/consumer style) queue" (c6396d9)

Just noticed this PR is creating a new src/core/ subdirectory to hold the queue and thread code. This seems good, but it may also be good to add a short core/README.md to say what the directory is supposed to be for. For example, if it's meant to hold utility code that isn't bitcoin specific, or if it might make sense in the future to move other code there.

ryanofsky · 2018-08-13T18:28:40Z

src/core/consumerthread.h

+
+private:
+    ShutdownPill(ConsumerThread<MODE>& consumer) : m_consumer(consumer){};
+    void operator()()


In commit "Add ConsumerThread: to consumer and operate on work from a ProducerConsumerQueue" (7f8a888)

Should this be marked "override?"

ryanofsky · 2018-08-13T18:31:04Z

src/core/consumerthread.h

+
+protected:
+    WorkItem(){};
+    virtual void operator()(){};


In commit "Add ConsumerThread: to consumer and operate on work from a ProducerConsumerQueue" (7f8a888)

It might be a good idea to make this abstract (= 0) to trigger a compile error in case a subclass declares this the wrong way and fails to override.

ryanofsky · 2018-08-13T19:10:20Z

src/core/producerconsumerqueue.h

+ *
+ * @see WorkerMode
+ */
+template <typename T, WorkerMode m_producer_mode = WorkerMode::BLOCKING, WorkerMode m_consumer_mode = WorkerMode::BLOCKING>


Re: #12934 (comment)

Another advantage to add to list above:

If you make blocking an attribute of push/pop methods rather than an attibutes of the queue you can drop the consumer/producer terminology, which I'm finding confusing now that I'm looking at downstream code. E.g. if I push an item into the queue, that seems like producing from my perspective, but it's consuming from the queue/worker thread perspective and makes that code a bit strange, IMO.

ryanofsky · 2018-08-13T19:35:35Z

src/core/consumerthread.h

+};
+
+template <WorkerMode PRODUCER_MODE>
+class WorkQueue : public BlockingConsumerQueue<std::unique_ptr<WorkItem<PRODUCER_MODE>>, PRODUCER_MODE>


In commit "Add ConsumerThread: to consumer and operate on work from a ProducerConsumerQueue" (7f8a888)

Calling this variable PRODUCER_MODE here, but passing it as the consumer_mode argument to BlockingConsumerQueue/ProducerConsumerQueue is a little unexpected. Could you maybe note this in a short comment to avoid confusion? Alternately it might be clearer just to use ProducerConsumerQueue directly, and not have BlockingConsumerQueue as a thing.

TBH, also, I don't actually understand why PRODUCER_MODE exists as a parameter. Why would code constructing WorkQueue want to control the default consumer mode of the queue when it doesn't consume from the queue (ConsumerThread does)?

ryanofsky · 2018-08-13T20:53:59Z

src/test/consumerthread_tests.cpp

+    }
+}
+
+BOOST_AUTO_TEST_CASE(foo)


In commit "ConsumerThread unit tests" (134d47a)

Should replace foo.

ryanofsky · 2018-08-13T20:57:16Z

src/test/consumerthread_tests.cpp

+
+    for (int i = 0; i < n_elements; i++) {
+        work[i] = i;
+        queue->Push(std::unique_ptr<TestWorkItem>(new TestWorkItem(work[i])));


In commit "ConsumerThread unit tests" (134d47a)

I think there is a race here between work[i] being assigned and then incremented in the worker thread. You could avoid it by setting work values after creating the vector but before creating the queue.

ryanofsky · 2018-08-13T21:01:20Z

src/test/consumerthread_tests.cpp

+
+    BOOST_CHECK_LT(queue->size(), n_threads + 1);
+    for (int i = 0; i < n_elements; i++) {
+        BOOST_CHECK_EQUAL(work[i], i + 2);


In commit "ConsumerThread unit tests" (134d47a)

Might be good to move this check above Terminate() to make the test more strict.

ryanofsky · 2018-08-13T21:27:18Z

src/core/consumerthread.h

+
+//! A special WorkItem() that is used to interrupt a blocked ConsumerThread() so that it can terminate
+template <WorkerMode MODE>
+class ShutdownPill : public WorkItem<MODE>


In commit "Add ConsumerThread: to consumer and operate on work from a ProducerConsumerQueue" (7f8a888)

I think I agree with James. Even if you want to support shutting down specific threads, an approach more like "Everybody wake up, check if you are supposed to exit, and if not go back to sleep again" might be simpler than "Random thread wake up, check if you are supposed to exit, and if not wake up another random thread, but not if you already seen this particular notification before," or whatever the correct description is. (If you do want to stick with the current approach, it could be nice to add a high level comment explaining it like this.)

ryanofsky · 2018-08-13T21:32:22Z

src/core/consumerthread.h

+
+            // if the same pill has been seen by the same thread previously then it can safely be discarded
+            // the intended thread has either terminated or is currently processing a work item and will terminate
+            // after completing that item and before blocking on the queue


In commit "Add ConsumerThread: to consumer and operate on work from a ProducerConsumerQueue" (7f8a888)

Why couldn't the intended thread just be blocked calling Pop() and not terminated or currently processing anything? It seems like this is assuming threads are notified in a circular order.

ryanofsky · 2018-08-14T14:54:22Z

src/validation_layer.h

+    void Stop();
+
+    //! Submit a block for asynchronous validation
+    std::future<BlockValidationResponse> SubmitForValidation(const std::shared_ptr<const CBlock> block, bool force_processing, std::function<void()> on_ready = []() {});


In commit "ValidationLayer() - interface for calls into block validation" (b471728)

Would be slightly more efficient to make default on_ready value nullptr instead of a no-op lambda.

It's also kind of unclear what m_ready is supposed to be used for in this context. You might want to move down your other comment from m_on_ready about c++11 futures here.

ryanofsky · 2018-08-14T20:20:28Z

src/validation_layer.cpp

+
+void BlockValidationRequest::operator()()
+{
+    LogPrint(BCLog::VALIDATION, "%s: validating request=%s\n", __func__, GetId());


In commit "ValidationLayer() - interface for calls into block validation" (b471728)

Maybe add the word "block" somewhere in here to make it clear what this is validating and obvious this is a block hash.

ryanofsky · 2018-08-14T20:23:46Z

src/validation_layer.cpp

+
+void ValidationLayer::Start()
+{
+    assert(!m_thread || !m_thread->IsActive());


In commit "ValidationLayer() - interface for calls into block validation" (b471728):

I think it would be better to just assert !m_thread to simplify and be more conservative. If m_thread is allowed to be non-null, then I think you would need to add more synchronization here to make sure join is called before the thread is destroyed to prevent a crash in the destructor: https://en.cppreference.com/w/cpp/thread/thread/%7Ethread

ryanofsky · 2018-08-14T20:42:38Z

src/core/consumerthread.h

+    }
+
+    //! Waits until this thread terminates
+    //! RequestTerminate() must have been previously called or be called by a different thread


In commit "Add ConsumerThread: to consumer and operate on work from a ProducerConsumerQueue" (7f8a888)

I think it would be clearer to just say this will block until RequestTerminate() is called. It should be perfectly fine to call RequestTerminate before or after this call and from any thread.

ryanofsky · 2018-08-14T20:45:18Z

src/validation_layer.cpp

+
+std::future<BlockValidationResponse> ValidationLayer::SubmitForValidation(const std::shared_ptr<const CBlock> block, bool force_processing, std::function<void()> on_ready)
+{
+    BlockValidationRequest* req = new BlockValidationRequest(*this, block, force_processing, on_ready);


In commit "ValidationLayer() - interface for calls into block validation" (b471728)

Would be safer / more efficient to use std::make_shared here.

ryanofsky · 2018-08-14T20:54:03Z

src/validation_layer.h

+    const std::shared_ptr<ValidationQueue> m_validation_queue;
+
+    //! the validation thread - sequentially processes validation requests from m_validation_queue
+    std::unique_ptr<ValidationThread> m_thread;


In commit "ValidationLayer() - interface for calls into block validation" (b471728)

Can this just be a ValidationThread instead of a pointer to one? The extra indirection doesn't seem helpful.

ryanofsky · 2018-08-14T20:57:25Z

src/validation_layer.h

+public:
+    ValidationLayer(const CChainParams& chainparams)
+        : m_chainparams(chainparams), m_validation_queue(std::make_shared<ValidationQueue>(100)) {}
+    ~ValidationLayer(){};


In commit "ValidationLayer() - interface for calls into block validation" (b471728)

Maybe assert thread is not joinable here, to help debugging in case this is not shut down correctly.

ryanofsky · 2018-08-15T16:27:47Z

src/init.cpp

    // using the other before destroying them.
    if (peerLogic) UnregisterValidationInterface(peerLogic.get());
    if (g_connman) g_connman->Stop();
+    if (g_validation_layer) g_validation_layer->Stop();


In commit "Call ProcessNewBlock() asynchronously in a separate thread from p2p layer" (1c9f741)

Should this also free g_validation_layer? (or have a comment saying why it shouldn't be freed)

ryanofsky · 2018-08-15T17:11:58Z

src/net.cpp

+            bool request_was_queued = pnode->IsAwaitingInternalRequest();
+
+            // If an internal request was queued and it's not done yet, skip this node
+            if (request_was_queued && !pnode->ProcessInternalRequestResults(m_msgproc))


In commit "Call ProcessNewBlock() asynchronously in a separate thread from p2p layer" (1c9f741)

I think this code might be clearer if the IsAwaitingInternalRequest were call were dropped and ProcessInternalRequestResults just returned requested_was_queued directly. It seems awkward how IsAwaitingInternalRequest and ProcessInternalRequestResults are checking some of the same things and then this code is combining their return values.

ryanofsky · 2018-08-15T17:21:30Z

src/test/test_bitcoin.h


 #include <boost/thread.hpp>

+class ValidationLayer;


In commit "Call ProcessNewBlock() asynchronously in a separate thread from p2p layer" (1c9f741)

Probably remove, doesn't look like this is used right now.

ryanofsky · 2018-08-15T17:32:48Z

src/net_processing.cpp

 }

-bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv, int64_t nTimeReceived, const CChainParams& chainparams, CConnman* connman, const std::atomic<bool>& interruptMsgProc, bool enable_bip61)
+    bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv, int64_t nTimeReceived, const CChainParams& chainparams, CConnman* connman, ValidationLayer& validation_layer, const std::atomic<bool>& interruptMsgProc, bool enable_bip61)


In commit "Call ProcessNewBlock() asynchronously in a separate thread from p2p layer" (1c9f741)

Unintended indent?

ryanofsky · 2018-08-15T17:46:36Z

src/net_processing.cpp

+    // process from some other peer.  We do this after calling
+    // ProcessNewBlock so that a malleated cmpctblock announcement
+    // can't be used to interfere with block relay.
+    if (!pindex || pindex->IsValid(BLOCK_VALID_TRANSACTIONS)) {


In commit "Call ProcessNewBlock() asynchronously in a separate thread from p2p layer" (1c9f741)

Maybe drop the !pindex check here, or say in a comment whether this would ever be expected? It seems surprising to treat null pindex like the block is valid.

ryanofsky · 2018-08-15T17:49:01Z

src/net_processing.cpp

+        MarkBlockAsReceived(pblock->GetHash());
+    }
+
+    if (validation_response.is_new) {


In commit "Call ProcessNewBlock() asynchronously in a separate thread from p2p layer" (1c9f741)

This seems ok, but I wanted to note that previous code in the fBlockReconstructed case updated nLastBlockTime/mapBlockSource before calling MarkBlockAsReceived, instead of after.

ryanofsky · 2018-08-15T17:55:49Z

src/net_processing.cpp

                // out to be invalid.
                mapBlockSource.emplace(resp.blockhash, std::make_pair(pfrom->GetId(), false));
            }
        } // Don't hold cs_main when we call into ProcessNewBlock


In commit "Call ProcessNewBlock() asynchronously in a separate thread from p2p layer" (1c9f741)

Should s/ProcessNewBlock/SubmitBlock/

ryanofsky · 2018-08-15T20:10:11Z

src/net_processing.cpp

                // though the block was successfully read, and rely on the
                // handling in ProcessNewBlock to ensure the block index is
                // updated, reject messages go out, etc.
-                MarkBlockAsReceived(resp.blockhash); // it is now an empty pointer


In commit "Call ProcessNewBlock() asynchronously in a separate thread from p2p layer" (1c9f741)

I guess with this line removed, the block will be marked received later, from the worker thread, after it is processed. This seems ok, though I could see why you might want to mark the block received when its received but before it's processed, so it's clearer what "received" and "processed" actually refer to.

ryanofsky · 2018-08-16T19:32:49Z

src/net.cpp

+
+void CNode::SetPendingInternalRequest(const std::shared_ptr<const CBlock> block, std::future<BlockValidationResponse>&& pending_response, const CBlockIndex* pindex)
+{
+    m_block_validating = block;


In commit "Call ProcessNewBlock() asynchronously in a separate thread from p2p layer" (1c9f741)

Would it be possible to assert m_block_validating variables are null/invalid here before overwriting them? It seems like it would help debugging if SetPendingInternalRequest were called for a new request before the previous request completed.

ryanofsky · 2018-08-16T19:40:20Z

src/validation_layer.cpp

+ * @param[out]  fNewBlock A boolean which is set to indicate if the block was first received via this call
+ * @return True if state.IsValid()
+ */
+bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr<const CBlock> pblock, bool fForceProcessing, bool* fNewBlock);


In commit "Limit available scope of ProcessNewBlock to ValidationLayer (move-only)" (3778736)

It's unusual that ProcessNewBlock is documented and declared in validation_layer.cpp but defined in src/validation.cpp. It seems like it will make the documentation hard to find. There are a bunch of other options that seem like they would be better to me:

Moving ProcessNewBlock declaration to validate_layer.cpp but moving the documentation to validate.cpp near the definition.

Leaving ProcessNewBlock where it is but renaming it to ProcessNewBlock internal.

Making ProcessNewBlock a private method of a class that is friends with ValidationLayer

In commit "Limit available scope of ProcessNewBlock to ValidationLayer (move-only)" (3778736)

LOCKS_EXCLUDED(cs_main) annotation seems to have been dropped here.

ryanofsky · 2018-08-16T19:56:01Z

src/test/test_bitcoin.cpp

-            if (!ActivateBestChain(state, chainparams)) {
-                throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", FormatStateMessage(state)));
-            }
+    // Ideally we'd move all the RPC tests to the functional testing framework


In commit "Fix whitespace in test_bitcoin.cpp (whitespace,move-only)" (3d6f038)

Maybe drop "move-only" from commit description, since this is actually just a whitespace change.

practicalswift · 2018-09-02T08:09:01Z

src/core/consumerthread.h

+
+                // resubmit it so that it gets a chance to get to the right thread
+                // when resubmitting, do not block and do not care about failures
+                // theres a potential deadlock where we try to push this to a queue thats


Typo found by codespell: “theres” should be “there is” :-)

Typo found by codespell: “thats” should be “that is” :-)

practicalswift · 2018-09-02T08:09:18Z

src/core/producerconsumerqueue.h

+
+        T ret;
+
+        // use a temporary so theres no side effecting code inside an assert which could be disabled


Typo found by codespell: “theres” should be “there is” :-)

DrahtBot · 2018-12-03T16:19:03Z

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

3339ba2 Make g_enable_bip61 a member variable of PeerLogicValidation (Jesse Cohen) 6690a28 Restrict as much as possible in net_processing to translation unit (Jesse Cohen) 1d4df02 [move-only] Move things only referenced in net_processing out of header file (Jesse Cohen) 02bbc05 Rescope g_enable_bip61 to net_processing (Jesse Cohen) Pull request description: As part of a larger effort to decouple net_processing and validation a bit, these are a bunch of simple scope cleanups. I've moved things out of the header file that are only referenced in net_processing and added static (or anonymous namespace) modifiers to everything possible in net_processing. There are a handful of functions which could be static except that they are exposed for the sake of unit testing - these are explicitly commented. There has been some discussion of a compile time annotation, but no conclusion has been reached on that yet. This is somewhat related to other prs bitcoin#12934 bitcoin#13413 bitcoin#13407 and will be followed by prs that reduce reliance on cs_main to synchronize data structures which are translation unit local to net_processing Tree-SHA512: 46c9660ee4e06653feb42ba92189565b0aea17aac2375c20747c0d091054c63829cbf66d2daddf65682b58ce1d6922e23aefea051a7f2c8abbb6db253a609082 Signed-off-by: Pasta <[email protected]> # Conflicts: # src/init.cpp # src/net_processing.cpp # src/net_processing.h # src/test/test_dash.cpp

skeees mentioned this pull request Apr 17, 2018

p2p_compactblocks.py failing occasionally on master #12978

Closed

ryanofsky reviewed Apr 23, 2018

View reviewed changes

skeees force-pushed the module-isolation branch 5 times, most recently from 01d79ea to 0798b88 Compare June 4, 2018 23:15

DrahtBot mentioned this pull request Jun 5, 2018

rpc: Avoid "duplicate" return value for invalid submitblock #13395

Closed

skeees force-pushed the module-isolation branch 4 times, most recently from c3bd4ee to bcab2cf Compare June 5, 2018 11:35

skeees changed the title ~~[WIP] [net] [validation] Call ProcessNewBlock() asynchronously~~ [net] [validation] Call ProcessNewBlock() asynchronously Jun 5, 2018

DrahtBot mentioned this pull request Jun 5, 2018

rpc: Add submitheader #13399

Merged

skeees mentioned this pull request Jun 6, 2018

[refactor, move-only-ish] Refactor mempool accept/reject logic #13407

Closed

DrahtBot mentioned this pull request Jun 7, 2018

Document validationinterace callback blocking deadlock potential. #13402

Merged

This was referenced Jun 7, 2018

[net,mempool] Call AcceptToMemoryPool() asynchronously in p2p #13413

Closed

[net] Tighten scope in net_processing #13417

Merged

This was referenced Jun 14, 2018

rpc: Avoid "duplicate" return value for invalid submitblock #13439

Merged

[bugfix] Fix encoding issue for Windows #13426

Closed

DrahtBot added the Needs rebase label Jun 15, 2018

ryanofsky reviewed Jun 19, 2018

View reviewed changes

skeees force-pushed the module-isolation branch 2 times, most recently from efdb111 to 2f52e4d Compare June 19, 2018 19:15

DrahtBot added the Needs rebase label Aug 8, 2018

ryanofsky reviewed Aug 13, 2018

View reviewed changes

ryanofsky reviewed Aug 14, 2018

View reviewed changes

ryanofsky reviewed Aug 15, 2018

View reviewed changes

ryanofsky reviewed Aug 16, 2018

View reviewed changes

practicalswift reviewed Sep 2, 2018

View reviewed changes

HashUnlimited mentioned this pull request Sep 11, 2018

[net] Tighten scope in net_processing chaincoin/chaincoin#517

Merged

TheBlueMatt mentioned this pull request Sep 22, 2018

Unbounded growth of scheduler queue #14289

Open

DrahtBot added the Up for grabs label Dec 3, 2018

DrahtBot closed this Dec 3, 2018

This was referenced Jun 9, 2019

Add a new peer state tracking class to reduce cs_main contention. #16174

Closed

Call ProcessNewBlock() asynchronously #16175

Closed

TheBlueMatt mentioned this pull request Jul 2, 2019

Call ProcessNewBlock() asynchronously #16323

Closed

laanwj removed the Needs rebase label Oct 24, 2019

dongcarl mentioned this pull request May 12, 2020

[WIP] rebase: Call ProcessNewBlock() asynchronously #18963

Closed

2 tasks

fanquake removed the Up for grabs label May 13, 2020

jnewbery mentioned this pull request Aug 6, 2020

Move remaining application layer data to net processing #19398

Closed

16 tasks

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022


		T ret;

		// use a temporary so theres no side effecting code inside an assert which could be disabled

[net] [validation] Call ProcessNewBlock() asynchronously #12934

[net] [validation] Call ProcessNewBlock() asynchronously #12934

Uh oh!

Conversation

skeees commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Update

Description

Uh oh!

fanquake commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skeees commented Apr 11, 2018

Uh oh!

instagibbs commented Apr 13, 2018

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skeees Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skeees commented Apr 23, 2018

Uh oh!

TheBlueMatt commented Apr 28, 2018

Uh oh!

skeees commented Jun 5, 2018

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skeees commented Apr 10, 2018 •

edited

Loading

fanquake commented Apr 11, 2018 •

edited

Loading

ryanofsky left a comment •

edited

Loading

skeees Apr 23, 2018 •

edited

Loading

ryanofsky left a comment •

edited

Loading

ryanofsky Jun 19, 2018 •

edited

Loading

ryanofsky Jul 3, 2018 •

edited

Loading

ryanofsky Aug 15, 2018 •

edited

Loading