💬 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).
(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?
(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)
(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.
(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
...
(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
...
(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.
(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.
(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);
}
```
(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)
(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.
(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.
(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.
(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
...
(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.
(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())`
(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.
(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.
💬 ryanofsky commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474750408)
In commit "sqlite: Ensure that only one SQLiteBatch is writing to db at a time" (e7ab074cb0d07dd9a16015702d55491413e286bb)
Semaphore is leaked here if m_db is null. Would suggest:
```c++
if (!m_database.m_db || m_txn) return false;
m_database.m_sqlite_semaphore.wait();
```
I would also suggest dropping the `m_database.HasActiveTxn()` check because it is confusing (it should never be true after the semaphore is acquired).
If there really is an active transaction it is better to tri
...
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474750408)
In commit "sqlite: Ensure that only one SQLiteBatch is writing to db at a time" (e7ab074cb0d07dd9a16015702d55491413e286bb)
Semaphore is leaked here if m_db is null. Would suggest:
```c++
if (!m_database.m_db || m_txn) return false;
m_database.m_sqlite_semaphore.wait();
```
I would also suggest dropping the `m_database.HasActiveTxn()` check because it is confusing (it should never be true after the semaphore is acquired).
If there really is an active transaction it is better to tri
...
💬 ryanofsky commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474738411)
In commit "sqlite: Ensure that only one SQLiteBatch is writing to db at a time" (e7ab074cb0d07dd9a16015702d55491413e286bb)
Would suggest the same comment here. // Acquire the write semaphore if it was not previously acquired while creating a transaction.
Separately, it seems like SQLiteBatch::WriteKey and SQLiteBatch::ExecStatement could be deduplicated or consolidated. They are basically identical copies of the same code.
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474738411)
In commit "sqlite: Ensure that only one SQLiteBatch is writing to db at a time" (e7ab074cb0d07dd9a16015702d55491413e286bb)
Would suggest the same comment here. // Acquire the write semaphore if it was not previously acquired while creating a transaction.
Separately, it seems like SQLiteBatch::WriteKey and SQLiteBatch::ExecStatement could be deduplicated or consolidated. They are basically identical copies of the same code.
💬 ryanofsky commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474780571)
In commit "sqlite: Ensure that only one SQLiteBatch is writing to db at a time" (e7ab074cb0d07dd9a16015702d55491413e286bb)
Similar comments as for TxnCommit(), would suggest just writing `if (!m_database.m_db || !m_txn) return false;` and dropping the `HasActiveTxn()` check which should never be false if m_txn is true.
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474780571)
In commit "sqlite: Ensure that only one SQLiteBatch is writing to db at a time" (e7ab074cb0d07dd9a16015702d55491413e286bb)
Similar comments as for TxnCommit(), would suggest just writing `if (!m_database.m_db || !m_txn) return false;` and dropping the `HasActiveTxn()` check which should never be false if m_txn is true.