💬 maflcko commented on pull request "addrman: improve performance of `GetAddr` when specifying network":
(https://github.com/bitcoin/bitcoin/pull/29578#issuecomment-2066381384)
> opinion

Performance improvements should be measured in numbers by machines, which does not seem like an opinion. According to https://github.com/bitcoin/bitcoin/pull/29578#issuecomment-2023247803 , the code can only be reached by RPC and P2P. For those places no end-to-end performance difference has been provided. I'd say if there is a measurable (reproducible) performance improvement for a users' use case, or another use case that hasn't been mentioned yet, the change can be considered bas
...

💬 furszy commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1572234194)
tiny typo
```suggestion
// Only genesis, which must be part of the best header chain, can have a nullptr parent.
```

💬 stratospher commented on issue "DNS seed "seed.bitcoinstats.com" doesn't support filtering while the comments says it does":
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2066393593)
oh interesting! there's some discussion in https://github.com/bitcoin/bitcoin/pull/29809 about how to update the comments for DNS seed filtering. also cc @naiyoma

💬 kcalvinalvin commented on issue "DNS seed "seed.bitcoinstats.com" doesn't support filtering while the comments says it does":
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2066402231)
I guess I should add that I don't think the nodes returned by "seed.bitcoinstats.com" are good.

My assumptions is that it's just caching all the nodes it's previously seen and not checking if they're still providing all the services it previously has.

⚠️ maflcko opened an issue: "RFC: Formal description of the RPC API"
(https://github.com/bitcoin/bitcoin/issues/29912)
Currently (all?) clients manually implement the RPC API, which is problematic, because:

* It leads to accidental implementation bugs, such as unit mistakes (s/vB vs BTC/kvB)
* It is hard to maintain, when the API changes
* It requires effort to implement in a new programming language

So it could make sense to have a formal specification.

Please leave a comment if there are additional aspects to consider here, or if you have ideas for a solution.

One solution for a formal descriptio
...

💬 nflatrea commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2066435250)
The example you showed doesn't even support C/C++ code-base.
If you need clear documentation / specification on the JSON-RPC API for bitcoincore you have plethora of well documented guides.

https://developer.bitcoin.org/reference/rpc/
https://wbnns.github.io/bitcoin-dev-docs/reference/rpc/index.html
https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md

https://en.bitcoin.it/wiki/API_reference_%28JSON-RPC%29

Considering your need for a state-of-the-art framework / w
...

📝 furszy opened a pull request: "bugfix: update chainman best_header after block reconsideration"
(https://github.com/bitcoin/bitcoin/pull/29913)
Fixes #26245. I'm doing it because it came to my mind after reviewing #28339.

Inside the `invalidateblock` process, we update the best_header by manually calling `InvalidChainFound` after disconnecting blocks.

We need to do the same for `reconsiderblock` and update the chain `best_header` field after finishing the process.

Note: the only difference between this two commands is that `reconsiderblock` does not have its own separate function, it's a plain RPC command that resets the block
...

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1572288041)
Added this test and tweaked a bit to check that orphan stays + can still be resolved.

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1572288118)
done

💬 levantah commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2066470976)
> Possibly the best estimate we can get for that is to look at full-RBF replacements that do not get mined. It's not too uncommon to see, for example, OpenTimestamp's full-RBF replacements mysteriously get mined at the second or third highest fee-rate they were propagated at even though multiple minutes have passed since the higher fee-rate txs were broadcast. I believe that is evidence of poor propagation to miners on occasion.

Just a reminder that there is still this "too many replacements"
...

💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#issuecomment-2066473350)
Ok ready for review again.

💬 maflcko commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2066477567)
> Don't we have plethora of well documented guides and specifications on the JSON-RPC api ?

No. I'd highly doubt that there is a better source of the documentation and specification than Bitcoin Core itself. (If there is, then that is a bug in Bitcoin Core, which should be fixed)

> https://developer.bitcoin.org/reference/rpc/

This example has many issues:

* It is not machine readable
* It does not indicate the version of Bitcoin Core that is documented
* It is wrong and outdated. F
...

💬 maflcko commented on pull request "bugfix: update chainman best_header after block reconsideration":
(https://github.com/bitcoin/bitcoin/pull/29913#discussion_r1572306247)
Conceptually, this seems the wrong place for the fix. It would be nice if this was handled internally in chainman correctly.

Also, given that cs_main is held only for this line, a race exits, where the `getchainstates` RPC is called before this line, and still returns the wrong result.

💬 furszy commented on pull request "bugfix: update chainman best_header after block reconsideration":
(https://github.com/bitcoin/bitcoin/pull/29913#discussion_r1572336276)
> Conceptually, this seems the wrong place for the fix. It would be nice if this was handled internally in chainman correctly.

I did that initially (see #26260 - https://github.com/bitcoin/bitcoin/pull/26260#issuecomment-1270121139) but.. conceptually, all what is inside `reconsiderblock` seems to be at the wrong place for me too. We are resetting a block index flag at the RPC level. And, ideally, we should encapsulate all this code within chainman too.

Also, see https://github.com/bitcoin
...

💬 hebasto commented on pull request "[WIP] build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2066565398)
> We can cherry-pick one commit from upstream leveldb, make the same change in crc32c, and then ultimately drop our build infra for testing endianness.

The same goal, which is dropping "build infra for testing endianness", might be achieved with an [alternative](https://github.com/hebasto/bitcoin/commit/ee5096ac4e638f088c78850a5d2e401fbe88537f) approach, which essentially boils down to:
```diff
--- a/src/leveldb/util/coding.h
+++ b/src/leveldb/util/coding.h
@@ -62,7 +62,7 @@ char* EncodeV
...

👍 vasild approved a pull request: "net: attempts to connect to all resolved addresses when connecting to a node"
(https://github.com/bitcoin/bitcoin/pull/28834#pullrequestreview-2011406209)
ACK fd81a37239541d0d508402cd4eeb28f38128c1f2

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1572363409)
pushed updated text

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1572364608)
added a small check here using `GetEntry` that takes `Wtxid`s.

💬 fanquake commented on pull request "[WIP] build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2066586806)
I don't think that's better. We have to keep all the redundant code, and now we are diveraging from upstream for no reason.

💬 BrandonOdiwuor commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-2066602344)
@maflcko @hernanmarino sorry I misunderstood https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-1956896591 earlier

When the wallet is disabled in the configure, the wallet RPCs are not included in the `rpc_interface.txt` list, hence not part of the coverage check


(example below from my test)
[rpc_interface.txt](https://github.com/bitcoin/bitcoin/files/15041102/rpc_interface.txt)