👍 Xekyo approved a pull request: "wallet: Refactor and document CoinControl"
(https://github.com/bitcoin/bitcoin/pull/26066#pullrequestreview-1410939913)
ACK daba95700b0b77a2e898299f218c47a69ed2c7d0
(https://github.com/bitcoin/bitcoin/pull/26066#pullrequestreview-1410939913)
ACK daba95700b0b77a2e898299f218c47a69ed2c7d0
💬 Xekyo commented on pull request "wallet: Refactor and document CoinControl":
(https://github.com/bitcoin/bitcoin/pull/26066#discussion_r1183713583)
Nit: If it’s not too much of a bother, and if you have to touch the code again, I would consider it an improvement to change `UnSelect` to `Unselect`. The first time I scanned that passage, the capitalized “S” in the middle of the word made me think that it was actually just “Select”. Even more nitty, I’d use the adjective “unselected” to refer to things that were not picked in the first place, I would prefer “to deselect” to refer to removing things from a selection.
```suggestion
void CCo
...
(https://github.com/bitcoin/bitcoin/pull/26066#discussion_r1183713583)
Nit: If it’s not too much of a bother, and if you have to touch the code again, I would consider it an improvement to change `UnSelect` to `Unselect`. The first time I scanned that passage, the capitalized “S” in the middle of the word made me think that it was actually just “Select”. Even more nitty, I’d use the adjective “unselected” to refer to things that were not picked in the first place, I would prefer “to deselect” to refer to removing things from a selection.
```suggestion
void CCo
...
💬 Xekyo commented on pull request "wallet: Refactor and document CoinControl":
(https://github.com/bitcoin/bitcoin/pull/26066#discussion_r1183714494)
Nit with the same reasoning as above:
```suggestion
void CCoinControl::DeselectAll()
```
(https://github.com/bitcoin/bitcoin/pull/26066#discussion_r1183714494)
Nit with the same reasoning as above:
```suggestion
void CCoinControl::DeselectAll()
```
💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183738245)
> I wonder if the for loop can be removed, and replaced by the shutdown workflow from https://github.com/bitcoin/bitcoin/pull/26674#issuecomment-1345237799
That does sound more sane than continuously re-indexing forever, or waiting for the user to interrupt.
The change I pushed will be reverted. How about instead of moving the previously `static` functions of `blockstorage.cpp` into its `BlockManager` class, moving them to a stateless sub-class of the `BlockManager`? Then we can have the
...
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183738245)
> I wonder if the for loop can be removed, and replaced by the shutdown workflow from https://github.com/bitcoin/bitcoin/pull/26674#issuecomment-1345237799
That does sound more sane than continuously re-indexing forever, or waiting for the user to interrupt.
The change I pushed will be reverted. How about instead of moving the previously `static` functions of `blockstorage.cpp` into its `BlockManager` class, moving them to a stateless sub-class of the `BlockManager`? Then we can have the
...
💬 MarcoFalke commented on pull request "tests: Run both descriptor and legacy tests within a single test invocation":
(https://github.com/bitcoin/bitcoin/pull/20892#discussion_r1183754343)
> They're necessary for https://github.com/bitcoin/bitcoin/commit/88b63a6e163045e63a7540427834a57799234407 "tests: Automatically run both wallet types in parallel for wallet tests".
I looked at that commit and it looks like the test list for the tests to run in parallel is created before passing it to `TestHandler`. `TestHandler` should never create conflicting/duplicate datadirs, because each test has its own directory based on the global test_runner directore, the name of the test, and its
...
(https://github.com/bitcoin/bitcoin/pull/20892#discussion_r1183754343)
> They're necessary for https://github.com/bitcoin/bitcoin/commit/88b63a6e163045e63a7540427834a57799234407 "tests: Automatically run both wallet types in parallel for wallet tests".
I looked at that commit and it looks like the test list for the tests to run in parallel is created before passing it to `TestHandler`. `TestHandler` should never create conflicting/duplicate datadirs, because each test has its own directory based on the global test_runner directore, the name of the test, and its
...
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183761759)
I very much like them to be stateful, see also https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183556059 . This is also needed for some other, unrelated, stuff that someone should be working on (cc @josibake )
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183761759)
I very much like them to be stateful, see also https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183556059 . This is also needed for some other, unrelated, stuff that someone should be working on (cc @josibake )
💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183762778)
This cleanup applies to other functions as well, so I am happy to have this split up, to avoid this pull from growing too large?
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183762778)
This cleanup applies to other functions as well, so I am happy to have this split up, to avoid this pull from growing too large?
💬 furszy commented on pull request "wallet: Load database records in a particular order":
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1183773763)
In f76830e5:
This db cursor changes need to be in the first commit as it's not compiling currently.
(https://github.com/bitcoin/bitcoin/pull/24914#discussion_r1183773763)
In f76830e5:
This db cursor changes need to be in the first commit as it's not compiling currently.
💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1183775132)
You need to make sure that you get *at least* 100 unique coins, otherwise the below loop can do `auto prevout = available_coins.begin();` on empty set, causing undefined behavior(duplicate inputs, in my case).
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1183775132)
You need to make sure that you get *at least* 100 unique coins, otherwise the below loop can do `auto prevout = available_coins.begin();` on empty set, causing undefined behavior(duplicate inputs, in my case).
👍 fanquake approved a pull request: "ci: Use arm_container.dockerfile"
(https://github.com/bitcoin/bitcoin/pull/27562#pullrequestreview-1411049490)
nice ACK fa6e2bfd0570324c80677d894247936bc151b4f8
(https://github.com/bitcoin/bitcoin/pull/27562#pullrequestreview-1411049490)
nice ACK fa6e2bfd0570324c80677d894247936bc151b4f8
💬 achow101 commented on pull request "wallet: Replace `GetBalance()` logic with `AvailableCoins()`":
(https://github.com/bitcoin/bitcoin/pull/26756#issuecomment-1533154837)
Closing due to lack of interest in this approach.
(https://github.com/bitcoin/bitcoin/pull/26756#issuecomment-1533154837)
Closing due to lack of interest in this approach.
✅ achow101 closed a pull request: "wallet: Replace `GetBalance()` logic with `AvailableCoins()`"
(https://github.com/bitcoin/bitcoin/pull/26756)
(https://github.com/bitcoin/bitcoin/pull/26756)
💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1183794878)
hmm I think the fuzzer was smart enough to give a coin with the same hash as the txid of a constructed transaction below, built on a different set of ancestors...
I think hashing these bytes would defeat this since it can't generate a valid tx with that txid?
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1183794878)
hmm I think the fuzzer was smart enough to give a coin with the same hash as the txid of a constructed transaction below, built on a different set of ancestors...
I think hashing these bytes would defeat this since it can't generate a valid tx with that txid?
👍 hebasto approved a pull request: "ci: Use arm_container.dockerfile"
(https://github.com/bitcoin/bitcoin/pull/27562#pullrequestreview-1411070769)
ACK fa6e2bfd0570324c80677d894247936bc151b4f8
(https://github.com/bitcoin/bitcoin/pull/27562#pullrequestreview-1411070769)
ACK fa6e2bfd0570324c80677d894247936bc151b4f8
💬 achow101 commented on pull request "test: add test for corrupt wallet bdb logs":
(https://github.com/bitcoin/bitcoin/pull/20974#issuecomment-1533164196)
I'm not sure that the issue this is trying to fix is really an issue, especially since we have BDB slated for removal.
(https://github.com/bitcoin/bitcoin/pull/20974#issuecomment-1533164196)
I'm not sure that the issue this is trying to fix is really an issue, especially since we have BDB slated for removal.
💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1183797662)
Fixed to `!=`
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1183797662)
Fixed to `!=`
💬 achow101 commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1183797967)
Added a commit dropping those.
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1183797967)
Added a commit dropping those.
💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183797966)
I was unclear with the word "stateless". What I meant was, "does not retain memory of previous interactions", e.g. we set all the options (including the consensus params) on initialization.
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1183797966)
I was unclear with the word "stateless". What I meant was, "does not retain memory of previous interactions", e.g. we set all the options (including the consensus params) on initialization.
💬 ryanofsky commented on pull request "Add fee_est tool for debugging fee estimation code":
(https://github.com/bitcoin/bitcoin/pull/10443#issuecomment-1533184415)
> is there some special information in the log you are writing that we can't get from the debug log file?
I think so. The json log captures all inputs to fee estimator to used update its internal state. It includes not just mempool events but also information about which transactions are included in blocks, flush events, and contents of `fee_estimates.dat` blobs that are loaded. It makes fee estimation code reproducible, so if you make a change to the fee estimation code or parameters, you ca
...
(https://github.com/bitcoin/bitcoin/pull/10443#issuecomment-1533184415)
> is there some special information in the log you are writing that we can't get from the debug log file?
I think so. The json log captures all inputs to fee estimator to used update its internal state. It includes not just mempool events but also information about which transactions are included in blocks, flush events, and contents of `fee_estimates.dat` blobs that are loaded. It makes fee estimation code reproducible, so if you make a change to the fee estimation code or parameters, you ca
...
💬 achow101 commented on pull request "tests: Run both descriptor and legacy tests within a single test invocation":
(https://github.com/bitcoin/bitcoin/pull/20892#discussion_r1183812824)
Sorry, I think I gave the wrong commit. It should be a936a3f194f855e269a4dc2c25e17dbc55eb7a18 "tests: Run both descriptor and legacy wallet modes in single invocation".
The tmpdir changes are needed for this commit because the entire test is restarted for the other wallet type, so it needs to be able to make a new datadir.
(https://github.com/bitcoin/bitcoin/pull/20892#discussion_r1183812824)
Sorry, I think I gave the wrong commit. It should be a936a3f194f855e269a4dc2c25e17dbc55eb7a18 "tests: Run both descriptor and legacy wallet modes in single invocation".
The tmpdir changes are needed for this commit because the entire test is restarted for the other wallet type, so it needs to be able to make a new datadir.