👍 fanquake approved a pull request: "RFC: Deprecate libconsensus"
(https://github.com/bitcoin/bitcoin/pull/29189#pullrequestreview-1856895919)
ACK 25dc87e6f84c38c21e109e11f7bbd93f1e1f3183 - this library has very little, if any impactful real world usage. It has been entirely broken (on various platforms) for long periods of its existence, where nobody even noticed. Pruning this out to save porting, and starting anew with the kernel, is the sane thing to do.

🚀 fanquake merged a pull request: "RFC: Deprecate libconsensus"
(https://github.com/bitcoin/bitcoin/pull/29189)

✅ fanquake closed a pull request: "libconsensus: adapt API header to be compliant to ANSI C"
(https://github.com/bitcoin/bitcoin/pull/28661)

💬 fanquake commented on pull request "libconsensus: adapt API header to be compliant to ANSI C":
(https://github.com/bitcoin/bitcoin/pull/28661#issuecomment-1921685192)
Going to close this for now, given the deprecation / plan to remove (#29189).

💬 fanquake commented on pull request "build: Remove HAVE_CONSENSUS_LIB":
(https://github.com/bitcoin/bitcoin/pull/29123#issuecomment-1921697022)
Could turn this PR into a full removal for 28.x? Otherwise I guess close, and we can followup with that in future?

✅ TheCharlatan closed a pull request: "build: Remove HAVE_CONSENSUS_LIB"
(https://github.com/bitcoin/bitcoin/pull/29123)

💬 TheCharlatan commented on pull request "build: Remove HAVE_CONSENSUS_LIB":
(https://github.com/bitcoin/bitcoin/pull/29123#issuecomment-1921707277)
Seems cleaner to close and do the removal in a separate one. Might be good to open a PR for the full removal soon, so it can get enough review before branch-off.

💬 fanquake commented on issue "build warnings in outputtype.cpp: may be used uninitialized":
(https://github.com/bitcoin/bitcoin/issues/29359#issuecomment-1921734117)
> I wonder why g++ on other distros does not print those warnings.

I see the same if I use a similar GCC on a different distro, i.e Alpine 3.18 with GCC `g++ (Alpine 12.2.1_git20220924-r10) 12.2.1 20220924)`:
```bash
CXX libbitcoin_common_a-protocol.o
In file included from ./hash.h:13,
from ./pubkey.h:10,
from ./addresstype.h:9,
from ./outputtype.h:9,
from outputtype.cpp:6:
In member function 'bool prevector<N
...

📝 starius opened a pull request: "Extend signetchallenge to set target block spacing"
(https://github.com/bitcoin/bitcoin/pull/29365)
Inspired by https://github.com/bitcoin/bitcoin/pull/27446, this PR implements the proposal detailed in the comment https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1516600820.

## Rationale

Introduce the ability to configure a custom target time between blocks in a custom Bitcoin signet network. This enhancement enables users to create a signet that is more conducive to testing. The change enhances the flexibility of signet, rendering it more versatile for various testing scenario
...

💬 furszy commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1474722573)
In 3f0f43d60:

A topic about this change:

`GetBlockChecked()` internally checks the block proof of work and the signet signature after reading the data from disk. The new `GetRawBlockChecked()` does not perform this checks.

It isn't an issue for me because the block wouldn't have been stored if the PoW was invalid and I don't think that `getblock` is the appropriate place to check for corruption or tampered data.

💬 furszy commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1474760569)
In 3d7ae60f21:

Same discussion as with the previous commit. `ReadBlockFromDisk()` checks PoW and the signet signature after reading the block from disk while `ReadRawBlockFromDisk()` doesn't.

I agree on not performing these checks but would be nice to describe the changes in the commit description.

💬 furszy commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1474716665)
In 3f0f43d605b44:

could inline it:
```c++
if (verbosity <= 0) {
return HexStr(block_data);
}
```

💬 furszy commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1474711807)
As we only care about the data existence here, what about doing a more general check:
```c++
if (!(blockindex.nStatus & BLOCK_HAVE_DATA)) {
throw JSONRPCError(RPC_MISC_ERROR, "Block not available");
}
```

(Unless you want to keep the same error message as before)

💬 fanquake commented on pull request "[WIP] C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1921757432)
Could rebase, and update the PR description? Ideally this is now just blocked on our supported compiler versions.

💬 sipa commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1474796055)
Yes, you'd need to make it an array of std::byte.

🤔 ryanofsky reviewed a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1856825883)
Code review fe617be10860cb5108399192ca65d85dc270622d. Looks good but I think there is still a race in SQLiteBatch::Close and a semaphore leak in SQLiteBatch::TxnBegin. Still reviewing, mostly just looked at the first commit so far.

💬 ryanofsky commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474700824)
In commit "sqlite: Ensure that only one SQLiteBatch is writing to db at a time" (e7ab074cb0d07dd9a16015702d55491413e286bb)

Suggest adding a comment about how this variable is used /** Boolean tracking whether this batch object has started a active transaction and whether it owns SqliteDatabase::m_write_semaphore. If the batch object starts a transaction, it acquires the semaphore and sets this to true, keeping the semaphore until the transaction ends to prevent other batch objects from writin
...

💬 ryanofsky commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474677032)
In commit "sqlite: Ensure that only one SQLiteBatch is writing to db at a time" (e7ab074cb0d07dd9a16015702d55491413e286bb)

Would suggest calling this `m_write_semaphore` not `m_sqlite_semaphore` to be clear it is only needed for writing the database.

💬 ryanofsky commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474773127)
In commit "sqlite: Ensure that only one SQLiteBatch is writing to db at a time" (e7ab074cb0d07dd9a16015702d55491413e286bb)

Would suggest just writing `if (!m_database.m_db || !m_txn) return false;` and dropping the `HasActiveTxn()` check.

`HasActiveTxn` should never be false if `m_txn` is true, and if it is, it is better to trigger the "Failed to commit transaction" log print and return false than to silently return false. Or alternately, this could `assert(m_database.HasActiveTxn())`

💬 ryanofsky commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474734221)
In commit "sqlite: Ensure that only one SQLiteBatch is writing to db at a time" (e7ab074cb0d07dd9a16015702d55491413e286bb)

Would suggest adding a comment like // Acquire the write semaphore if it was not previously acquired while creating a transaction.