💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480406547)
This test is modified in the last commit; it'd be more convincingly improving runtime if the final version of the test was already included in the commit that initially adds it.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480406547)
This test is modified in the last commit; it'd be more convincingly improving runtime if the final version of the test was already included in the commit that initially adds it.
🤔 sr-gi reviewed a pull request: "test: maxuploadtarget: check for mempool msg disconnect if limit is reached, improve existing test coverage"
(https://github.com/bitcoin/bitcoin/pull/28996#pullrequestreview-1866063353)
tack [b58f009](https://github.com/bitcoin/bitcoin/commit/b58f009d9585aab775998644de07e27e2a4a8045)
(https://github.com/bitcoin/bitcoin/pull/28996#pullrequestreview-1866063353)
tack [b58f009](https://github.com/bitcoin/bitcoin/commit/b58f009d9585aab775998644de07e27e2a4a8045)
💬 sr-gi commented on pull request "test: maxuploadtarget: check for mempool msg disconnect if limit is reached, improve existing test coverage":
(https://github.com/bitcoin/bitcoin/pull/28996#discussion_r1480385304)
This is sort of unrelated, but it may be worth bringing it in given we are updating this test anyway. The line right above this mentions that the time is advanced by 24 hours and then the checks are performed again. However, the time is actually advance 48 hours: it was previously set to 48 hours in the past, and now it's set to the current time.
I guess it wouldn't hurt to also define `SECONDS_PER_DAY = 60*60*24` for readability throughout the test.
(https://github.com/bitcoin/bitcoin/pull/28996#discussion_r1480385304)
This is sort of unrelated, but it may be worth bringing it in given we are updating this test anyway. The line right above this mentions that the time is advanced by 24 hours and then the checks are performed again. However, the time is actually advance 48 hours: it was previously set to 48 hours in the past, and now it's set to the current time.
I guess it wouldn't hurt to also define `SECONDS_PER_DAY = 60*60*24` for readability throughout the test.
💬 sr-gi commented on pull request "test: maxuploadtarget: check for mempool msg disconnect if limit is reached, improve existing test coverage":
(https://github.com/bitcoin/bitcoin/pull/28996#discussion_r1480406396)
nit: whitespace + caps here
(https://github.com/bitcoin/bitcoin/pull/28996#discussion_r1480406396)
nit: whitespace + caps here
💬 mzumsande commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1480436573)
> do you have a code path where nChainTx is read when it was wrong (previously) or is unset (now)
no, I never understood how the fake `nChainTx` values would help with guessing progress, but also wasn't 100% sure.
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1480436573)
> do you have a code path where nChainTx is read when it was wrong (previously) or is unset (now)
no, I never understood how the fake `nChainTx` values would help with guessing progress, but also wasn't 100% sure.
💬 ryanofsky commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1930671864)
> This leads to an alternative solution to not increase the memory footprint: Move the 32+32 bits used for nTx and nChainTx into a single 64-bit blob, where the 16 bit are for nTx and the remaining for nChainTx. But, again, the overall heap memory usage shouldn't change much, either way.
This does seem like an nice and easy solution. Would be good to incorporate in #29331
As a tangent, I was thinking along the same lines last week before implementing #29370 and came up with worse solution,
...
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1930671864)
> This leads to an alternative solution to not increase the memory footprint: Move the 32+32 bits used for nTx and nChainTx into a single 64-bit blob, where the 16 bit are for nTx and the remaining for nChainTx. But, again, the overall heap memory usage shouldn't change much, either way.
This does seem like an nice and easy solution. Would be good to incorporate in #29331
As a tangent, I was thinking along the same lines last week before implementing #29370 and came up with worse solution,
...
💬 epiccurious commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1480491658)
Question on your approach here - why drop the `try .. catch ... if not raised` instead of just replacing line 27?
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1480491658)
Question on your approach here - why drop the `try .. catch ... if not raised` instead of just replacing line 27?
💬 epiccurious commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#issuecomment-1930691538)
Concept ACK 950f3602d61f51d3c42c051ed9139916f1ee23bd.
(https://github.com/bitcoin/bitcoin/pull/29393#issuecomment-1930691538)
Concept ACK 950f3602d61f51d3c42c051ed9139916f1ee23bd.
💬 brunoerg commented on pull request "i2p: log connection was refused due to arbitrary port":
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1480499623)
Because they are not necessary anymore. That try catch was a hack to check the logs because we didn't have a direct way to check in the logs that the reason was the port.
(https://github.com/bitcoin/bitcoin/pull/29393#discussion_r1480499623)
Because they are not necessary anymore. That try catch was a hack to check the logs because we didn't have a direct way to check in the logs that the reason was the port.
🤔 furszy reviewed a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1866399622)
ACK cfcb9b1
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1866399622)
ACK cfcb9b1
💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480562396)
In f770aff87279ba252f7b8fcadef0e9d1828ca031 "test: Add coin_grinder_tests"
This check implies that there is an exact solution at 2041 attempts, and we should be able to know what that solution is, so can we check for it?
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480562396)
In f770aff87279ba252f7b8fcadef0e9d1828ca031 "test: Add coin_grinder_tests"
This check implies that there is an exact solution at 2041 attempts, and we should be able to know what that solution is, so can we check for it?
💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480563895)
In 3e2a97bd4bb275e2f0827a3099a64222c6460ed9 "coinselection: Track whether CG completed"
style nit: Open brace `{` should be on a new line for functions.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480563895)
In 3e2a97bd4bb275e2f0827a3099a64222c6460ed9 "coinselection: Track whether CG completed"
style nit: Open brace `{` should be on a new line for functions.
💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480554649)
In f770aff87279ba252f7b8fcadef0e9d1828ca031 "test: Add coin_grinder_tests"
nit: A comment with the weight calculation like the ones in later test would be nice.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480554649)
In f770aff87279ba252f7b8fcadef0e9d1828ca031 "test: Add coin_grinder_tests"
nit: A comment with the weight calculation like the ones in later test would be nice.
💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480558134)
In f770aff87279ba252f7b8fcadef0e9d1828ca031 "test: Add coin_grinder_tests"
I think it would be useful to clarify that `expected_attempts` is the iteration limit.
The comment suggests that the check should be `<=` but the check looks for exactly this number of attempts. That seems fragile if there are further optimizations made that improve this.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480558134)
In f770aff87279ba252f7b8fcadef0e9d1828ca031 "test: Add coin_grinder_tests"
I think it would be useful to clarify that `expected_attempts` is the iteration limit.
The comment suggests that the check should be `<=` but the check looks for exactly this number of attempts. That seems fragile if there are further optimizations made that improve this.
💬 achow101 commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480564307)
In 04a043a973602bc74043f7c5767bcd55246913cb "coinselection: Add CoinGrinder algorithm"
style nit: Open brace `{` should be on a new line for functions.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1480564307)
In 04a043a973602bc74043f7c5767bcd55246913cb "coinselection: Add CoinGrinder algorithm"
style nit: Open brace `{` should be on a new line for functions.
💬 achow101 commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1930854540)
ACK 600fc562f815941b4a95c9f4f357dc6eed04be6c
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1930854540)
ACK 600fc562f815941b4a95c9f4f357dc6eed04be6c
👍 kristapsk approved a pull request: "i2p: log connection was refused due to arbitrary port"
(https://github.com/bitcoin/bitcoin/pull/29393#pullrequestreview-1866436287)
cr utACK 950f3602d61f51d3c42c051ed9139916f1ee23bd
(https://github.com/bitcoin/bitcoin/pull/29393#pullrequestreview-1866436287)
cr utACK 950f3602d61f51d3c42c051ed9139916f1ee23bd
💬 theuni commented on issue "doc: List identifiers used from header in #if(def)":
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1930911069)
I've pushed a branch here that does a large cleanup: https://github.com/theuni/bitcoin/commits/cleanup-config-h-headers/
The approach is to include `bitcoin-config.h` in _any_ file which uses a configure define, even if it would have been included by an earlier header. That gives us a safe starting point for further removals.
My approach was somewhat manual, but I think it's trustworthy:
Grab all possibly defined tokens: `grep undef config/bitcoin-config.h.in | cut -d" " -f2 | grep -v '
...
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1930911069)
I've pushed a branch here that does a large cleanup: https://github.com/theuni/bitcoin/commits/cleanup-config-h-headers/
The approach is to include `bitcoin-config.h` in _any_ file which uses a configure define, even if it would have been included by an earlier header. That gives us a safe starting point for further removals.
My approach was somewhat manual, but I think it's trustworthy:
Grab all possibly defined tokens: `grep undef config/bitcoin-config.h.in | cut -d" " -f2 | grep -v '
...
💬 mzumsande commented on pull request "test: make v2transport arg in addconnection mandatory and few cleanups":
(https://github.com/bitcoin/bitcoin/pull/29356#discussion_r1480649475)
well, after actually looking what the tests does it doesn't matter either way, since we don't actually make a real connection here...
(https://github.com/bitcoin/bitcoin/pull/29356#discussion_r1480649475)
well, after actually looking what the tests does it doesn't matter either way, since we don't actually make a real connection here...
👍 ryanofsky approved a pull request: "test: Add makefile target for running unit tests"
(https://github.com/bitcoin/bitcoin/pull/29377#pullrequestreview-1866665176)
Tested ACK 5ca9b24da18e842e7a093dc44f6b222af73e92cf.
This is very convenient but the check-unit target does not seem to depend on the `test_bitcoin` executable, so it can run with a stale test binary.
I also noticed if I try to build and run the unit tests with `make -j12 -C src test/test_bitcoin check-unit` it will try to run the tests at the same time as building the test binary instead of waiting for the binary to be built.
I also wonder if this new `check-unit` test option is redund
...
(https://github.com/bitcoin/bitcoin/pull/29377#pullrequestreview-1866665176)
Tested ACK 5ca9b24da18e842e7a093dc44f6b222af73e92cf.
This is very convenient but the check-unit target does not seem to depend on the `test_bitcoin` executable, so it can run with a stale test binary.
I also noticed if I try to build and run the unit tests with `make -j12 -C src test/test_bitcoin check-unit` it will try to run the tests at the same time as building the test binary instead of waiting for the binary to be built.
I also wonder if this new `check-unit` test option is redund
...