💬 theStack commented on pull request "test: Support decoding segwit address in address_to_scriptpubkey()":
(https://github.com/bitcoin/bitcoin/pull/27269#discussion_r1146099889)
@josibake: yes, agree that considering on what we need in `address_to_scriptpubkey` returning `(None, None)` is better than asserting. LGTM!

👍 theStack approved a pull request: "test: Support decoding segwit address in address_to_scriptpubkey()"
(https://github.com/bitcoin/bitcoin/pull/27269)
ACK d178082996dc3000f42816f89afcf3fa4d31e159 ✔️

> I'd love to see a follow-up PR targeting areas where we are a) only testing base58 addresses but should be testing bech32 as well and b) anywhere we are making a getaddressinfo RPC call to get the spk to instead use address_to_scriptpubkey

👍 If anyone wants to work on this, feel free to tag me as reviewer.

💬 instagibbs commented on pull request "rpc: Add submit option to generateblock":
(https://github.com/bitcoin/bitcoin/pull/18933#issuecomment-1481098679)
ACK fa18504d5767a40dc9827fb081633219bf251001

💬 furszy commented on pull request "wallet: return error msg for "too-long-mempool-chain"":
(https://github.com/bitcoin/bitcoin/pull/24845#discussion_r1146122336)
Nop. Could also have zero conf change (or send-to-self) if the wallet's `m_spend_zero_conf_change` flag is disabled. In other words, unconfirmed coins accepted by the mempool limits (ancestors/descendants < 25).

💬 stickies-v commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1481163868)
> It would be better to have two separate commits - one with the non-functional optimization and one with the fix.

Agreed, that could probably be improved.

> Idea: I think it would be better to disallow the creation of HTTPRequest if the request is invalid/unparsable.

This was the first approach I considered, but I think it's better not to. A request with an invalid URI is still a request, and thus it should be representable. Moving the URI validation to the constructor feels like a sho
...

👍 vasild approved a pull request: "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg"
(https://github.com/bitcoin/bitcoin/pull/27257)
ACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e

🚀 fanquake merged a pull request: "ci: Use clang-15 in "tidy" task"
(https://github.com/bitcoin/bitcoin/pull/27311)

💬 beirut-boop commented on pull request "wallet: unify “allow/block other inputs“ concept":
(https://github.com/bitcoin/bitcoin/pull/25118#issuecomment-1481195546)
Noticing some new irrational behavior in a forked coin control and this is the most significant work on coin control pulled from upstream. @furszy any change you could glance over the screenshots and assess the probability of this being an upstream issue?

https://github.com/syscoin/syscoin/issues/524

💬 furszy commented on pull request "reduce cs_main scope, guard block index 'nFile' under a local mutex":
(https://github.com/bitcoin/bitcoin/pull/27006#discussion_r1146202462)
> Shouldn't these be fetched atomically? Maybe there should be a getter that returns all three values, similar to how there's a setter (`CBlockIndex::SetFileData`) than sets all three atomically.

@LarryRuane I didn't add it merely to not introduce that many changes but could be done too. The members set is guarded by `cs_main` (for now) so there is no possible race.

> Starting to feel like a struct for those 3 members would be appropriate

Why exactly?. So far in the code, we either want
...

👍 theStack approved a pull request: "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg"
(https://github.com/bitcoin/bitcoin/pull/27257)
re-ACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e

👍 brunoerg approved a pull request: "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg"
(https://github.com/bitcoin/bitcoin/pull/27257)
re-ACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e

💬 fanquake commented on pull request "wallet: unify “allow/block other inputs“ concept":
(https://github.com/bitcoin/bitcoin/pull/25118#issuecomment-1481215346)
Sorry, this is not a support forum for forks of this project.

🚀 fanquake merged a pull request: "rpc: Add submit option to generateblock"
(https://github.com/bitcoin/bitcoin/pull/18933)

💬 fanquake commented on pull request "test: getblock on header throws":
(https://github.com/bitcoin/bitcoin/pull/27237#issuecomment-1481216289)
#18933 is now in.

💬 dergoegge commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1146221073)
fwiw, I would prefer either one of these two options over what is currently in this PR:
> * Use findBlock from the node interface. The overhead should only be another map lookup by the block hash? You can ask Ryan to confirm.
> * Pass down a blockman ref into the zmq stuff (massive change)

I implemented the second option here: https://github.com/dergoegge/bitcoin/commit/a17844fe3e3c97c0eb5906536b96d1ce634b790c feel free to pick that, it's not to crazy of a change imo (haven't tested that bu
...

💬 furszy commented on pull request "reduce cs_main scope, guard block index 'nFile' under a local mutex":
(https://github.com/bitcoin/bitcoin/pull/27006#issuecomment-1481222148)
> Concept ACK
>
> Is my understanding correct that 1) replacing `cs_main` with `g_cs_blockindex_data` and 2) having `g_cs_blockindex_data` be a shared instead of recursive/exclusive mutex are orthogonal improvements? I think it makes sense for both of them to happen in this PR, just wondering.

Moving `cs_main` to an blockstorage local exclusive mutex would still be an improvement from the current state, yeah. The networking threads (and pretty much the entire node) shouldn't get blocked be
...

💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1481229146)
> Moving the URI validation to the constructor feels like a shortcut and coupling we don't need to take?

I do not have a strong opinion. This PR moved the parsing to the constructor, but left the checking of the parsing result for later. Now the code looks like (simplified):

```cpp
// constructs the object, does the parsing, leaves null m_uri_parsed if it fails
HTTPRequest hreq(...);

if (hreq.m_uri_parsed == nullptr) {
handle error and dispose hreq
}

// elsewhere a soft asser
...

💬 furszy commented on pull request "wallet: unify “allow/block other inputs“ concept":
(https://github.com/bitcoin/bitcoin/pull/25118#issuecomment-1481229795)
@beirut-boop check #26699.

💬 TheCharlatan commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1481231791)
Updated 3ceb8dde48fc12f4f16372f661a4cccf7104e194 -> 00e9b97f37e0bdf4c647236838c10b68b7ad5be3 ([splitSystemFs_6](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_6) -> [splitSystemFs_7](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemFs_6..splitSystemFs_7)):
* Addressed my [comment](https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1146057675) by moving the `_POSIX_C_SOURCE` handling to `f
...

💬 willcl-ark commented on issue "core stops to run with `Failed to read block` error":
(https://github.com/bitcoin/bitcoin/issues/27142#issuecomment-1481249345)
Hey @vincenzopalazzo just wanted to follow up here...

Was this something that you managed to fix, has it gone away on it's own, or are you still experiencing problems with it?