💬 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.
💬 ryanofsky commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474730162)
In commit "sqlite: Ensure that only one SQLiteBatch is writing to db at a time" (e7ab074cb0d07dd9a16015702d55491413e286bb)
Can't add a review comment there, but line 411 `if (m_database.HasActiveTxn())` above needs to be changed to `if (m_txn)` to prevent a race condition. The `SQLiteBatch::Close` function is called by the `SQLiteBatch` destructor so it called on every batch object, whether or not is a created a transaction. If a batch object is being destroyed in one thread while a batch obj
...
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1474730162)
In commit "sqlite: Ensure that only one SQLiteBatch is writing to db at a time" (e7ab074cb0d07dd9a16015702d55491413e286bb)
Can't add a review comment there, but line 411 `if (m_database.HasActiveTxn())` above needs to be changed to `if (m_txn)` to prevent a race condition. The `SQLiteBatch::Close` function is called by the `SQLiteBatch` destructor so it called on every batch object, whether or not is a created a transaction. If a batch object is being destroyed in one thread while a batch obj
...
💬 maflcko commented on pull request "refactor: Fix timedata includes":
(https://github.com/bitcoin/bitcoin/pull/29361#issuecomment-1921825207)
done
(https://github.com/bitcoin/bitcoin/pull/29361#issuecomment-1921825207)
done
💬 maflcko commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1474833093)
The member function `write` is the low-level interface for stream-like classes to accept a span of bytes. Normally, it shouldn't be used in common code, only internally in serialization.
If you want to serialize an object in Bitcoin Core, you can use the `<<` operator to write and `>>` to read.
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1474833093)
The member function `write` is the low-level interface for stream-like classes to accept a span of bytes. Normally, it shouldn't be used in common code, only internally in serialization.
If you want to serialize an object in Bitcoin Core, you can use the `<<` operator to write and `>>` to read.
💬 maflcko commented on issue "build warnings in outputtype.cpp: may be used uninitialized":
(https://github.com/bitcoin/bitcoin/issues/29359#issuecomment-1921867817)
> 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)`:
Interesting. `alpine:latest` with g++-13.2.1 looked fine for me: https://cirrus-ci.com/task/4994174227841024?logs=make#L5
However, on Ubuntu, g++-13.2.0 had some warnings: https://cirrus-ci.com/task/4712699251130368?logs=make#L173
Maybe they were fixed in 13.2.1?
(https://github.com/bitcoin/bitcoin/issues/29359#issuecomment-1921867817)
> 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)`:
Interesting. `alpine:latest` with g++-13.2.1 looked fine for me: https://cirrus-ci.com/task/4994174227841024?logs=make#L5
However, on Ubuntu, g++-13.2.0 had some warnings: https://cirrus-ci.com/task/4712699251130368?logs=make#L173
Maybe they were fixed in 13.2.1?
🤔 pablomartin4btc reviewed a pull request: "Modify command line help to show support for BIP21 URIs"
(https://github.com/bitcoin-core/gui/pull/752#pullrequestreview-1857147189)
lgtm, re ACK ede5014c445dcb40ddcfdede2c51236bbfe85f5e
(https://github.com/bitcoin-core/gui/pull/752#pullrequestreview-1857147189)
lgtm, re ACK ede5014c445dcb40ddcfdede2c51236bbfe85f5e
🤔 glozow reviewed a pull request: "log: Don't use scientific notation in log messages"
(https://github.com/bitcoin/bitcoin/pull/29254#pullrequestreview-1857153947)
lgtm ACK c819a83b4d83413a31323c1304664ee4c228ca29. can you update the PR description?
(https://github.com/bitcoin/bitcoin/pull/29254#pullrequestreview-1857153947)
lgtm ACK c819a83b4d83413a31323c1304664ee4c228ca29. can you update the PR description?
💬 kristapsk commented on pull request "log: Don't use scientific notation in log messages":
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1921915135)
> lgtm ACK [c819a83](https://github.com/bitcoin/bitcoin/commit/c819a83b4d83413a31323c1304664ee4c228ca29). can you update the PR description?
Updated.
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1921915135)
> lgtm ACK [c819a83](https://github.com/bitcoin/bitcoin/commit/c819a83b4d83413a31323c1304664ee4c228ca29). can you update the PR description?
Updated.
🤔 pablomartin4btc reviewed a pull request: "assumeutxo, rpc: Add 'start' parameter to loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/28659#pullrequestreview-1857215681)
I think it's up to you really, you could either use this PR, updating title & description (mentioning previous approach) or create a new one referring this one, but you would need to make `loadtxoutset` async now (no? :thinking:) so the other [2 new RPC commands](https://github.com/bitcoin/bitcoin/pull/28659#issuecomment-1774662703) (to be created) work/ interact well.
(https://github.com/bitcoin/bitcoin/pull/28659#pullrequestreview-1857215681)
I think it's up to you really, you could either use this PR, updating title & description (mentioning previous approach) or create a new one referring this one, but you would need to make `loadtxoutset` async now (no? :thinking:) so the other [2 new RPC commands](https://github.com/bitcoin/bitcoin/pull/28659#issuecomment-1774662703) (to be created) work/ interact well.
💬 pablomartin4btc commented on pull request "rpc: Do not wait for headers inside loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/29345#issuecomment-1921952510)
> > Curious about reasonable arguments to keep it though
>
> The only reason I can see to keep it, is when it is made infinite, and the RPC is made async.
You might consider this coming then #28620 (make `loadtxoutset` async), the linked fix could be updated soon I think.
(https://github.com/bitcoin/bitcoin/pull/29345#issuecomment-1921952510)
> > Curious about reasonable arguments to keep it though
>
> The only reason I can see to keep it, is when it is made infinite, and the RPC is made async.
You might consider this coming then #28620 (make `loadtxoutset` async), the linked fix could be updated soon I think.
💬 achow101 commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1921984879)
Post merge ACK 25dc87e6f84c38c21e109e11f7bbd93f1e1f3183
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1921984879)
Post merge ACK 25dc87e6f84c38c21e109e11f7bbd93f1e1f3183
💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-1922011219)
I moved the static constants into classes and renamed them a bit. Expanded the fuzzer to send multiple messages back and forth (like the BIP324 fuzzer).
I dropped the 1 hour wiggle room in certificate timestamps, because it adds complexity and I don't expect this to cause any issues in practice. A self-signed certificate is generated when the TemplateProvider loads, which is unlikely the same second someone connects to it.
The fuzzer found a bug where I forgot to defragment messages larger
...
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-1922011219)
I moved the static constants into classes and renamed them a bit. Expanded the fuzzer to send multiple messages back and forth (like the BIP324 fuzzer).
I dropped the 1 hour wiggle room in certificate timestamps, because it adds complexity and I don't expect this to cause any issues in practice. A self-signed certificate is generated when the TemplateProvider loads, which is unlikely the same second someone connects to it.
The fuzzer found a bug where I forgot to defragment messages larger
...