💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466829063)
These lines are dropped now.
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466829063)
These lines are dropped now.
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466829719)
I nuked Docker in favor of Podman. So far it seems to work. I dropped 7cdb75d572e825b4c0cf74a516f912f94807853f.
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466829719)
I nuked Docker in favor of Podman. So far it seems to work. I dropped 7cdb75d572e825b4c0cf74a516f912f94807853f.
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1466864661)
Was thinking about why this is ok. AcceptMultipleTransactions doesn't allow RBF, and future package RBF should work with this?
```suggestion
// The mempool or in-package parent cannot have any other in-mempool children.
// We don't need to handle the "single RBF" case as that is
// handled in ApplyV3Rules for single transaction packages.
// Future package RBF would conflict with the parent as well,
// making this check sufficien
...
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1466864661)
Was thinking about why this is ok. AcceptMultipleTransactions doesn't allow RBF, and future package RBF should work with this?
```suggestion
// The mempool or in-package parent cannot have any other in-mempool children.
// We don't need to handle the "single RBF" case as that is
// handled in ApplyV3Rules for single transaction packages.
// Future package RBF would conflict with the parent as well,
// making this check sufficien
...
🤔 mzumsande reviewed a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1844567036)
Code Review ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1844567036)
Code Review ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3
💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-1910905907)
@ajtowns, to try to steer this discussion in a more constructive direction, would it be possible for you to make a list of the biggest harms you believe this PR would cause if it were merged? And if you have ideas on how the harms you see could be avoided, while the problems I am trying to solve (see "Problems this PR attempts to solve" in the PR description) could be addressed, it would be really helpful to know about. Maybe the harms you see this PR causing and the problems this PR is trying t
...
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-1910905907)
@ajtowns, to try to steer this discussion in a more constructive direction, would it be possible for you to make a list of the biggest harms you believe this PR would cause if it were merged? And if you have ideas on how the harms you see could be avoided, while the problems I am trying to solve (see "Problems this PR attempts to solve" in the PR description) could be addressed, it would be really helpful to know about. Maybe the harms you see this PR causing and the problems this PR is trying t
...
💬 achow101 commented on pull request "During IBD, prune as much as possible until we get close to where we will eventually keep blocks":
(https://github.com/bitcoin/bitcoin/pull/20827#issuecomment-1910911055)
ACK d298ff8b62b2624ed390c8a2f905c888ffc956ff
(https://github.com/bitcoin/bitcoin/pull/20827#issuecomment-1910911055)
ACK d298ff8b62b2624ed390c8a2f905c888ffc956ff
💬 achow101 commented on issue "getdescriptorinfo returns unusable descriptor":
(https://github.com/bitcoin/bitcoin/issues/29320#issuecomment-1910919438)
The purpose of the `descriptor` result of `getdescriptorinfo` is to reflect a descriptor that basically matches the user's input, just without including any private keys. `listdescriptors` specifically returns normalized descriptors so that they can be imported elsewhere.
`getdescriptorinfo` could also return the normalized descriptor, but it intentionally does not do that for `descriptor`. I suppose a separate field containing the normalized form could be added.
(https://github.com/bitcoin/bitcoin/issues/29320#issuecomment-1910919438)
The purpose of the `descriptor` result of `getdescriptorinfo` is to reflect a descriptor that basically matches the user's input, just without including any private keys. `listdescriptors` specifically returns normalized descriptors so that they can be imported elsewhere.
`getdescriptorinfo` could also return the normalized descriptor, but it intentionally does not do that for `descriptor`. I suppose a separate field containing the normalized form could be added.
🚀 achow101 merged a pull request: "During IBD, prune as much as possible until we get close to where we will eventually keep blocks"
(https://github.com/bitcoin/bitcoin/pull/20827)
(https://github.com/bitcoin/bitcoin/pull/20827)
👍 kristapsk approved a pull request: "debugwindow: update session ID tooltip"
(https://github.com/bitcoin-core/gui/pull/788#pullrequestreview-1844619925)
ACK 3bf00e13609eefa6ddb11353519bb1aec2342513
(https://github.com/bitcoin-core/gui/pull/788#pullrequestreview-1844619925)
ACK 3bf00e13609eefa6ddb11353519bb1aec2342513
✅ achow101 closed a pull request: "test: Generate coverage report without running tests"
(https://github.com/bitcoin/bitcoin/pull/28772)
(https://github.com/bitcoin/bitcoin/pull/28772)
🤔 ryanofsky reviewed a pull request: "wallet: guard against dangling to-be-reverted db transactions"
(https://github.com/bitcoin/bitcoin/pull/29253#pullrequestreview-1838949232)
Code review d4ebac8381801d024b69d5f1f5a13246c61edbe4. This seems like a nice change that could make the wallet more reliable, and adds good cleanup and test coverage.
Did not ack just because I noticed a bug in SQLiteBatch::TxnBegin if `m_db` is null, but none of my other comments are too important, so feel free to ignore them.
re: https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1910567093
> In general, we don't have very robust error handling for when the database fails to r
...
(https://github.com/bitcoin/bitcoin/pull/29253#pullrequestreview-1838949232)
Code review d4ebac8381801d024b69d5f1f5a13246c61edbe4. This seems like a nice change that could make the wallet more reliable, and adds good cleanup and test coverage.
Did not ack just because I noticed a bug in SQLiteBatch::TxnBegin if `m_db` is null, but none of my other comments are too important, so feel free to ignore them.
re: https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1910567093
> In general, we don't have very robust error handling for when the database fails to r
...
💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1463377113)
In commit "sqlite: add ability to interrupt statements" (ca39d812e4766575e25721296fa2d43dfe7eba7e)
It would be good to define -999 as a constant so callers could check for this error. Or if there probably won't be a need for callers to do that, it would be clearer to return SQLITE_ERROR.
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1463377113)
In commit "sqlite: add ability to interrupt statements" (ca39d812e4766575e25721296fa2d43dfe7eba7e)
It would be good to define -999 as a constant so callers could check for this error. Or if there probably won't be a need for callers to do that, it would be clearer to return SQLITE_ERROR.
💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466931843)
In commit "sqlite: add ability to interrupt statements" (ca39d812e4766575e25721296fa2d43dfe7eba7e)
Would suggest calling this `SQliteExecBlocker`, or maybe `SQliteExecHook` or `SQliteTestHook` if you anticipate it being used more broadly for other things in the future. "ExecHandler" seems like the wrong name because the class itself isn't executing anything and doesn't call sqlite3_exec.
Would also suggest adding a comment saying this is used for testing, because otherwise it's not clear w
...
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466931843)
In commit "sqlite: add ability to interrupt statements" (ca39d812e4766575e25721296fa2d43dfe7eba7e)
Would suggest calling this `SQliteExecBlocker`, or maybe `SQliteExecHook` or `SQliteTestHook` if you anticipate it being used more broadly for other things in the future. "ExecHandler" seems like the wrong name because the class itself isn't executing anything and doesn't call sqlite3_exec.
Would also suggest adding a comment saying this is used for testing, because otherwise it's not clear w
...
💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466947379)
In commit "sqlite: introduce HasActiveTxn method" (5670a21f5ef28748b6b7242c03b82f39dcbc983f)
It would be nice to have a comment explaining the use of sqlite3_get_autocommit, since the name doesn't really explain what it is doing. Maybe "// sqlite3_get_autocommit returns true by default, and false if a transaction has begun and not been committed or rolled back."
Also, I noticed https://www.sqlite.org/c3ref/get_autocommit.html says "If another thread changes the autocommit status of the dat
...
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466947379)
In commit "sqlite: introduce HasActiveTxn method" (5670a21f5ef28748b6b7242c03b82f39dcbc983f)
It would be nice to have a comment explaining the use of sqlite3_get_autocommit, since the name doesn't really explain what it is doing. Maybe "// sqlite3_get_autocommit returns true by default, and false if a transaction has begun and not been committed or rolled back."
Also, I noticed https://www.sqlite.org/c3ref/get_autocommit.html says "If another thread changes the autocommit status of the dat
...
💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466966956)
In commit "sqlite: guard against dangling to-be-reverted db transactions" (508fda2fddfef0cefedde9777c914aec72adf7fb)
I found this comment confusing because I couldn't figure out when failing to abort a transaction would ever be expected to happen, and what the error handling strategy was supposed to be when it did happen. Would maybe suggest: "// If transaction cannot be aborted, it means there is a bug or there has been data corruption. Try to recover in this case by closing and reopening th
...
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466966956)
In commit "sqlite: guard against dangling to-be-reverted db transactions" (508fda2fddfef0cefedde9777c914aec72adf7fb)
I found this comment confusing because I couldn't figure out when failing to abort a transaction would ever be expected to happen, and what the error handling strategy was supposed to be when it did happen. Would maybe suggest: "// If transaction cannot be aborted, it means there is a bug or there has been data corruption. Try to recover in this case by closing and reopening th
...
💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466968974)
In commit "test: sqlite, add coverage for dangling to-be-reverted db txns" (d4ebac8381801d024b69d5f1f5a13246c61edbe4)
I think it would be a little more consistent with other code to call this variable "batch" instead of "handler." Also calling it handler gets a little confusing when code below sets a handler on the handler.
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466968974)
In commit "test: sqlite, add coverage for dangling to-be-reverted db txns" (d4ebac8381801d024b69d5f1f5a13246c61edbe4)
I think it would be a little more consistent with other code to call this variable "batch" instead of "handler." Also calling it handler gets a little confusing when code below sets a handler on the handler.
💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466956009)
In commit "sqlite: introduce HasActiveTxn method" (5670a21f5ef28748b6b7242c03b82f39dcbc983f)
Would be good to drop the parenthetical about autocommit mode here, since that's now an implementation detail of HasActiveTxn()
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466956009)
In commit "sqlite: introduce HasActiveTxn method" (5670a21f5ef28748b6b7242c03b82f39dcbc983f)
Would be good to drop the parenthetical about autocommit mode here, since that's now an implementation detail of HasActiveTxn()
💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466953925)
In commit "sqlite: introduce HasActiveTxn method" (5670a21f5ef28748b6b7242c03b82f39dcbc983f)
There appears to be a bug here. Previously it would return false if `m_db` is null. Now it will try to execute begin transaction. I think you need to keep the `!m_database.m_db` condition
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1466953925)
In commit "sqlite: introduce HasActiveTxn method" (5670a21f5ef28748b6b7242c03b82f39dcbc983f)
There appears to be a bug here. Previously it would return false if `m_db` is null. Now it will try to execute begin transaction. I think you need to keep the `!m_database.m_db` condition
👍 theStack approved a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1844474487)
Code-review ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3
Went through another review round and only found some minor nits, mostly naming suggestions. If you retouch (or follow-up material):
* should use multi-line imports (see style guidelines in `test/functional/README.md`)
* commit 382894c3acd2dbf3e4198814f547c75b6fb17706 has a leading space in the commit subject
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1844474487)
Code-review ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3
Went through another review round and only found some minor nits, mostly naming suggestions. If you retouch (or follow-up material):
* should use multi-line imports (see style guidelines in `test/functional/README.md`)
* commit 382894c3acd2dbf3e4198814f547c75b6fb17706 has a leading space in the commit subject
💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466950294)
nit: the name `self.peer` doesn't say a lot, maybe something like `self.key_material` would be more descriptive? (though it's strictly speaking more than that, as it contains also stuff derived from the keys like garbage terminators and session id)
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1466950294)
nit: the name `self.peer` doesn't say a lot, maybe something like `self.key_material` would be more descriptive? (though it's strictly speaking more than that, as it contains also stuff derived from the keys like garbage terminators and session id)