🚀 fanquake merged a pull request: "test: Replace 0xC0 constant"
(https://github.com/bitcoin/bitcoin/pull/27143)
(https://github.com/bitcoin/bitcoin/pull/27143)
👍 ryanofsky approved a pull request: "Convert ArgsManager::GetDataDir to a read-only function"
(https://github.com/bitcoin/bitcoin/pull/27073)
Code review ACK 0af17e161d751b15f90615800411f36e11b3fcde. Nice change that makes initialization more straightforward and gets rid of strange behavior. Only change since last review is adding a comment. I left some small suggestions, but they are not important.
---
I think part of the PR description https://github.com/bitcoin/bitcoin/pull/27073#issue-1579684533 is inaccurate.
> As part of the refactoring it introduces a slight behaviour change to `GetConfigFilePath`, which now always ret
...
(https://github.com/bitcoin/bitcoin/pull/27073)
Code review ACK 0af17e161d751b15f90615800411f36e11b3fcde. Nice change that makes initialization more straightforward and gets rid of strange behavior. Only change since last review is adding a comment. I left some small suggestions, but they are not important.
---
I think part of the PR description https://github.com/bitcoin/bitcoin/pull/27073#issue-1579684533 is inaccurate.
> As part of the refactoring it introduces a slight behaviour change to `GetConfigFilePath`, which now always ret
...
💬 ryanofsky commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1114423128)
In commit "util: make GetDataDir read-only & create datadir.." (0af17e161d751b15f90615800411f36e11b3fcde)
Would be good to move this comment from the `EnsureDataDir` declaration to the `EnsureDataDir` implementation. The comment is trying to explain the confusing `create_directories("wallets")` calls there, so probably more helpful next to those calls.
It would be good to have API documentation for the two new functions though. I would move the function descriptions you wrong in the commit
...
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1114423128)
In commit "util: make GetDataDir read-only & create datadir.." (0af17e161d751b15f90615800411f36e11b3fcde)
Would be good to move this comment from the `EnsureDataDir` declaration to the `EnsureDataDir` implementation. The comment is trying to explain the confusing `create_directories("wallets")` calls there, so probably more helpful next to those calls.
It would be good to have API documentation for the two new functions though. I would move the function descriptions you wrong in the commit
...
💬 jonatack commented on pull request "I2P network optimizations":
(https://github.com/bitcoin/bitcoin/pull/26837#issuecomment-1440556067)
> I'm still seeing the issue I reported with i2pd version 2.45.1 (0.9.57).
No longer seeing the previously daily issue since a few days with i2pd version 2.46.0 and 2.46.1.
(https://github.com/bitcoin/bitcoin/pull/26837#issuecomment-1440556067)
> I'm still seeing the issue I reported with i2pd version 2.45.1 (0.9.57).
No longer seeing the previously daily issue since a few days with i2pd version 2.46.0 and 2.46.1.
💬 achow101 commented on pull request "assumeutxo: background validation completion":
(https://github.com/bitcoin/bitcoin/pull/25740#issuecomment-1440601441)
re-ACK a00d0e78510d79bc9fc57ec622aec98a65efa8c0
(https://github.com/bitcoin/bitcoin/pull/25740#issuecomment-1440601441)
re-ACK a00d0e78510d79bc9fc57ec622aec98a65efa8c0
👍 brunoerg approved a pull request: "docs: add ramdisk guide for running tests on OSX"
(https://github.com/bitcoin/bitcoin/pull/27124)
utACK 2f84ad7b9e62dd710940c2f265b65973b94864d7
(https://github.com/bitcoin/bitcoin/pull/27124)
utACK 2f84ad7b9e62dd710940c2f265b65973b94864d7
💬 jonatack commented on pull request "docs: add ramdisk guide for running tests on OSX":
(https://github.com/bitcoin/bitcoin/pull/27124#issuecomment-1440636295)
ACK 2f84ad7b9e62dd710940c2f265b65973b94864d7
M1 Max, macOS 13.2
```bash
$ diskutil erasevolume HFS+ ramdisk $(hdiutil attach -nomount ram://8388608)
Started erase on disk6
Unmounting disk
Erasing
Initialized /dev/rdisk6 as a 4 GB case-insensitive HFS Plus volume
Mounting disk
Finished erase on disk6 (ramdisk)
$ diskutil info disk6 | grep -e Size
Disk Size: 4.3 GB (4294967296 Bytes) (exactly 8388608 512-Byte-Units)
Device Block Size: 512 Bytes
A
...
(https://github.com/bitcoin/bitcoin/pull/27124#issuecomment-1440636295)
ACK 2f84ad7b9e62dd710940c2f265b65973b94864d7
M1 Max, macOS 13.2
```bash
$ diskutil erasevolume HFS+ ramdisk $(hdiutil attach -nomount ram://8388608)
Started erase on disk6
Unmounting disk
Erasing
Initialized /dev/rdisk6 as a 4 GB case-insensitive HFS Plus volume
Mounting disk
Finished erase on disk6 (ramdisk)
$ diskutil info disk6 | grep -e Size
Disk Size: 4.3 GB (4294967296 Bytes) (exactly 8388608 512-Byte-Units)
Device Block Size: 512 Bytes
A
...
📝 ishaanam opened a pull request: "wallet: when a block is disconnected, update transactions that are no longer conflicted"
(https://github.com/bitcoin/bitcoin/pull/27145)
This implements a fix for #7315. Previously when a block was disconnected any transactions that were conflicting with transactions mined in that block were not updated to be marked as inactive. The fix implemented here is described on the [Bitcoin DevWiki](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking#idea-refresh-conflicted). A test which tested the previous behavior has also been updated.
(https://github.com/bitcoin/bitcoin/pull/27145)
This implements a fix for #7315. Previously when a block was disconnected any transactions that were conflicting with transactions mined in that block were not updated to be marked as inactive. The fix implemented here is described on the [Bitcoin DevWiki](https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking#idea-refresh-conflicted). A test which tested the previous behavior has also been updated.
💬 achow101 commented on pull request "validation: Improve error handling when VerifyDB dosn't finish successfully":
(https://github.com/bitcoin/bitcoin/pull/25574#issuecomment-1440651446)
ACK 0af16e7134459e0820ab95d751093876c1ec4c6d
(https://github.com/bitcoin/bitcoin/pull/25574#issuecomment-1440651446)
ACK 0af16e7134459e0820ab95d751093876c1ec4c6d
💬 jonatack commented on pull request "assumeutxo: background validation completion":
(https://github.com/bitcoin/bitcoin/pull/25740#issuecomment-1440655812)
In the pull description, could drop or update "Since it's dependent on the commits in https://github.com/bitcoin/bitcoin/pull/25667, I'm opening this as a draft."
(https://github.com/bitcoin/bitcoin/pull/25740#issuecomment-1440655812)
In the pull description, could drop or update "Since it's dependent on the commits in https://github.com/bitcoin/bitcoin/pull/25667, I'm opening this as a draft."
🚀 achow101 merged a pull request: "validation: Improve error handling when VerifyDB dosn't finish successfully"
(https://github.com/bitcoin/bitcoin/pull/25574)
(https://github.com/bitcoin/bitcoin/pull/25574)
💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114818128)
Don't know if transaction received through BIP152's `BLOCKTXN` should be `TryRemovingFromSet()`.
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114818128)
Don't know if transaction received through BIP152's `BLOCKTXN` should be `TryRemovingFromSet()`.
💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114821225)
If the "fast" (i.e 2s) has been reasoned out in the bip or the paper, the section can be referenced too, I think.
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114821225)
If the "fast" (i.e 2s) has been reasoned out in the bip or the paper, the section can be referenced too, I think.
💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114835166)
Can add a comment about what `TxReconciliationTracker::IsPeerNextToReconcileWith()` does, like assumptions on peers queue and phase we're in.
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114835166)
Can add a comment about what `TxReconciliationTracker::IsPeerNextToReconcileWith()` does, like assumptions on peers queue and phase we're in.
💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114866248)
Not sure this new comment still holds with the introduction of `MAX_SET_SIZE=3000`.
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114866248)
Not sure this new comment still holds with the introduction of `MAX_SET_SIZE=3000`.
💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114877866)
Correct the max set size (`MAX_SET_SIZE`) is enforced by `ShouldFanoutTo()` which happens before the call to `AddToSet()`, don't know if it could be more conservative with another check here.
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114877866)
Correct the max set size (`MAX_SET_SIZE`) is enforced by `ShouldFanoutTo()` which happens before the call to `AddToSet()`, don't know if it could be more conservative with another check here.
💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114886751)
Well we might have some small discrepancy in old code, doesn't seem to matter here.
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114886751)
Well we might have some small discrepancy in old code, doesn't seem to matter here.
💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114888417)
I think it's accurate with the current comment of warning about less frequent reconciliations introducing high transaction relay latency. Like some trade-off between bandwidth and 0confs UX to be aware, I would say.
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114888417)
I think it's accurate with the current comment of warning about less frequent reconciliations introducing high transaction relay latency. Like some trade-off between bandwidth and 0confs UX to be aware, I would say.
💬 ariard commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114890070)
Gotcha, just means the malicious outbound peer will never move forward its own reconciliation.
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1114890070)
Gotcha, just means the malicious outbound peer will never move forward its own reconciliation.
💬 Ayush170-Future commented on issue "listtransactions results order unkown":
(https://github.com/bitcoin/bitcoin/issues/17739#issuecomment-1440726908)
Hello!
Is this issue up for grab?
Also, if it is, could someone help specify what kind of documentation has to be updated?
I'd love to get started on this.
(https://github.com/bitcoin/bitcoin/issues/17739#issuecomment-1440726908)
Hello!
Is this issue up for grab?
Also, if it is, could someone help specify what kind of documentation has to be updated?
I'd love to get started on this.